[compiler][arm] Address TODO replacing macro with template [v8/v8 : main]

[Matthias Liedtke (Gerrit)]

Attention needed from Nico Hartmann

Matthias Liedtke added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Matthias Liedtke . resolved

@nicoha...@chromium.org: You asked me to change this in https://crrev.com/c//5103510/comment/511ee805_1200da52/, so you have now the honor of reviewing this. 😊

Open in Gerrit

Related details

Attention is currently required from:

• Nico Hartmann

Submit Requirements:

• Code-Owners
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: Ic2ac7e0462b51d44ca5706976513c82df2513448

Gerrit-Change-Number: 7651905

Gerrit-PatchSet: 1

Gerrit-Owner: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Reviewer: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Reviewer: Nico Hartmann <nicoha...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Nico Hartmann <nicoha...@chromium.org>

Gerrit-Comment-Date: Tue, 10 Mar 2026 13:25:11 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Nico Hartmann (Gerrit)]

Attention needed from Matthias Liedtke

Nico Hartmann voted and added 2 comments

Votes added by Nico Hartmann

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Nico Hartmann . resolved

Great cleanup.

LGTM % nit

File src/compiler/backend/arm/instruction-selector-arm.cc

Line 3079, Patchset 1 (Latest):template <typename AddPairwiseS, typename AddPairwiseU>

Nico Hartmann . unresolved

nit: It's not super obvious what those arguments are supposed to be. Maybe can you rename those to reflect better that these are the masks? (e.g. `SignedAddMask`/`UnsignedAddMask`) Alternatively we could craft a concept for masks, but it's not necessary (might be a bit of a hassle).

Open in Gerrit

Related details

Attention is currently required from:

• Matthias Liedtke

Submit Requirements:

• Code-Owners
• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: Ic2ac7e0462b51d44ca5706976513c82df2513448

Gerrit-Change-Number: 7651905

Gerrit-PatchSet: 1

Gerrit-Owner: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Reviewer: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Reviewer: Nico Hartmann <nicoha...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Comment-Date: Wed, 11 Mar 2026 09:05:26 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Matthias Liedtke (Gerrit)]

Matthias Liedtke voted and added 2 comments

Votes added by Matthias Liedtke

Commit-Queue

+2

2 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Matthias Liedtke . resolved

Thanks for the review!

File src/compiler/backend/arm/instruction-selector-arm.cc

Line 3079, Patchset 1:template <typename AddPairwiseS, typename AddPairwiseU>

Nico Hartmann . resolved

nit: It's not super obvious what those arguments are supposed to be. Maybe can you rename those to reflect better that these are the masks? (e.g. `SignedAddMask`/`UnsignedAddMask`) Alternatively we could craft a concept for masks, but it's not necessary (might be a bit of a hassle).

Matthias Liedtke

Renaming done. I don't think concepts will really be useful here, they probably will make it harder to read and I don't think we need the extra type safetc that they offer.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Owners
• Code-Review

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: Ic2ac7e0462b51d44ca5706976513c82df2513448

Gerrit-Change-Number: 7651905

Gerrit-PatchSet: 2

Gerrit-Owner: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Reviewer: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Reviewer: Nico Hartmann <nicoha...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Wed, 11 Mar 2026 09:27:25 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Nico Hartmann <nicoha...@chromium.org>

[V8 LUCI CQ (Gerrit)]

V8 LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: src/compiler/backend/arm/instruction-selector-arm.cc
Insertions: 5, Deletions: 5.

@@ -3076,32 +3076,32 @@
 #undef SIMD_VISIT_UNIMPL_BINOP
 #undef UNIMPLEMENTED_SIMD_BINOP_LIST
 
-template <typename AddPairwiseS, typename AddPairwiseU>
+template <typename SignedAddMask, typename UnsignedAddMask>
 bool TryEmitExtAddPairwise(InstructionSelector* selector, OpIndex node,
                            const Simd128BinopOp& add_op, NeonSize neon_size_s,
                            NeonSize neon_size_u) {
   ArmOperandGenerator g(selector);
   const Operation& left = selector->Get(add_op.left());
   const Operation& right = selector->Get(add_op.right());
-  if (left.Is<AddPairwiseS>() && selector->CanCover(node, add_op.left())) {
+  if (left.Is<SignedAddMask>() && selector->CanCover(node, add_op.left())) {
     selector->Emit(kArmVpadal | MiscField::encode(neon_size_s),
                    g.DefineSameAsFirst(node), g.UseRegister(add_op.right()),
                    g.UseRegister(left.input(0)));
     return true;
   }
-  if (left.Is<AddPairwiseU>() && selector->CanCover(node, add_op.left())) {
+  if (left.Is<UnsignedAddMask>() && selector->CanCover(node, add_op.left())) {
     selector->Emit(kArmVpadal | MiscField::encode(neon_size_u),
                    g.DefineSameAsFirst(node), g.UseRegister(add_op.right()),
                    g.UseRegister(left.input(0)));
     return true;
   }
-  if (right.Is<AddPairwiseS>() && selector->CanCover(node, add_op.right())) {
+  if (right.Is<SignedAddMask>() && selector->CanCover(node, add_op.right())) {
     selector->Emit(kArmVpadal | MiscField::encode(neon_size_s),
                    g.DefineSameAsFirst(node), g.UseRegister(add_op.left()),
                    g.UseRegister(right.input(0)));
     return true;
   }
-  if (right.Is<AddPairwiseU>() && selector->CanCover(node, add_op.right())) {
+  if (right.Is<UnsignedAddMask>() && selector->CanCover(node, add_op.right())) {
     selector->Emit(kArmVpadal | MiscField::encode(neon_size_u),
                    g.DefineSameAsFirst(node), g.UseRegister(add_op.left()),
                    g.UseRegister(right.input(0)));
```

Change information

Commit message:

[compiler][arm] Address TODO replacing macro with template

Change-Id: Ic2ac7e0462b51d44ca5706976513c82df2513448

Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7651905

Commit-Queue: Matthias Liedtke <mlie...@chromium.org>

Reviewed-by: Nico Hartmann <nicoha...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#105720}

Files:

• M src/compiler/backend/arm/instruction-selector-arm.cc

Change size: M

Delta: 1 file changed, 51 insertions(+), 40 deletions(-)

Branch: refs/heads/main

Submit Requirements:

• Code-Review: +1 by Nico Hartmann

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: merged

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: Ic2ac7e0462b51d44ca5706976513c82df2513448

Gerrit-Change-Number: 7651905

Gerrit-PatchSet: 3

Gerrit-Owner: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Reviewer: Matthias Liedtke <mlie...@chromium.org>

Gerrit-Reviewer: Nico Hartmann <nicoha...@chromium.org>

Gerrit-Reviewer: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>