[compiler][arm64] Add 64-bit UBFIZ and improve Turboshaft selection [v8/v8 : main]
Marco Vitale (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Darius Mercadier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 8 comments![]( "Open in Gerrit")
const Operation& lhs = selector->Get(shift->left());
if (uint64_t constant_left;
lhs.Is<Opmask::kWord64BitwiseAnd>() &&
selector->CanCover(index, shift->left()) &&
selector->MatchUnsignedIntegralConstant(shift->right(), &constant_left)) {
uint64_t shift_by = constant_left;
if (base::IsInRange(shift_by, 1, 63)) {
const WordBinopOp& bitwise_and = lhs.Cast<WordBinopOp>();
if (uint64_t constant_right; selector->MatchUnsignedIntegralConstant(
bitwise_and.right(), &constant_right)) {
uint64_t mask = constant_right;
uint32_t mask_width = base::bits::CountPopulation(mask);
uint32_t mask_msb = base::bits::CountLeadingZeros64(mask);
if ((mask_width != 0) && (mask_msb + mask_width == 64) &&
((shift_by + mask_width) < 64)) {
return false;
}
}
}
}So... what's the point of this?
if (uint32_t mask; lhs.Is<Opmask::kWord32ShiftLeft>() &&Add a comment explaining what you're doing / what you're matching.
if ((mask_width != 0) && (mask_width != 32) &&
(base::bits::CountLeadingZeros32(mask) + mask_width +
mask_trailing_zeros ==
32)) {Ok so if I understand correctly, this matches a integer that just has a "pack" of 1s, like `0b00001111100000`. Let's extract this to a helper, add a comment explaining what it does, and reuse if also in VisitWord64And.
if (int64_t shift_by;The rhs of `ShiftOp` should always be a int32.
if (uint64_t mask;
lhs.Is<Opmask::kWord64ShiftLeft>() &&
CanCover(node, bitwise_and.left()) &&
MatchUnsignedIntegralConstant(bitwise_and.right(), &mask)) {
uint64_t mask_width = base::bits::CountPopulation(mask);
uint64_t mask_trailing_zeros = base::bits::CountTrailingZeros64(mask);
if ((mask_width != 0) && (mask_width != 64) &&
(base::bits::CountLeadingZeros64(mask) + mask_width +
mask_trailing_zeros ==
64)) {
const ShiftOp& shift = lhs.Cast<ShiftOp>();
if (int64_t shift_by;
MatchSignedIntegralConstant(shift.right(), &shift_by) &&
static_cast<uint64_t>(shift_by & 0x3F) == mask_trailing_zeros) {
if (mask_width + mask_trailing_zeros >= 64) {
Emit(kArm64Lsl, g.DefineAsRegister(node), g.UseRegister(shift.left()),
g.UseImmediate64(static_cast<int32_t>(mask_trailing_zeros)));
} else {
// Select Ubfiz for And(Lsl(x, imm), mask) where the mask is
// contiguous and its trailing zeros match the shift.
Emit(kArm64Ubfiz, g.DefineAsRegister(node),
g.UseRegister(shift.left()),
g.TempImmediate(static_cast<int32_t>(mask_trailing_zeros)),
g.TempImmediate(static_cast<int32_t>(mask_width)));
}
return;
}
}
}Can we extract this to a helper and reuse it for both VisitWord64And and VisitWord32And? (if the result looks too ugly because of the small differences between the 2, feel free not to do it)
if (base::IsInRange(shift_by, 1, 63)) {I'd merge this into the `if` right above
Related details
- Marco Vitale
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marco Vitale (Gerrit)
Marco Vitale added 7 comments![]( "Open in Gerrit")
const Operation& lhs = selector->Get(shift->left());
if (uint64_t constant_left;
lhs.Is<Opmask::kWord64BitwiseAnd>() &&
selector->CanCover(index, shift->left()) &&
selector->MatchUnsignedIntegralConstant(shift->right(), &constant_left)) {
uint64_t shift_by = constant_left;
if (base::IsInRange(shift_by, 1, 63)) {
const WordBinopOp& bitwise_and = lhs.Cast<WordBinopOp>();
if (uint64_t constant_right; selector->MatchUnsignedIntegralConstant(
bitwise_and.right(), &constant_right)) {
uint64_t mask = constant_right;
uint32_t mask_width = base::bits::CountPopulation(mask);
uint32_t mask_msb = base::bits::CountLeadingZeros64(mask);
if ((mask_width != 0) && (mask_msb + mask_width == 64) &&
((shift_by + mask_width) < 64)) {
return false;
}
}
}
}So... what's the point of this?
Opsie, leftover, sorry! I was trying to force UBFIZ to see if i was doing something wrong in one of the previous iteration.
if (uint32_t mask; lhs.Is<Opmask::kWord32ShiftLeft>() &&Add a comment explaining what you're doing / what you're matching.
Commented the whole logic, both in here and in the 64 version. Thanks for the advice
if ((mask_width != 0) && (mask_width != 32) &&
(base::bits::CountLeadingZeros32(mask) + mask_width +
mask_trailing_zeros ==
32)) {Ok so if I understand correctly, this matches a integer that just has a "pack" of 1s, like `0b00001111100000`. Let's extract this to a helper, add a comment explaining what it does, and reuse if also in VisitWord64And.
Unfortunately the helper is overly verbose, and it needs to return width, msb, and trailing_zeros (or taking them by ref, that it is something that i usually don't like). I would stick to the current version, as is consistent, like line 2125. Wdyt?
The rhs of `ShiftOp` should always be a int32.
Done
if (uint64_t mask;
lhs.Is<Opmask::kWord64ShiftLeft>() &&
CanCover(node, bitwise_and.left()) &&
MatchUnsignedIntegralConstant(bitwise_and.right(), &mask)) {
uint64_t mask_width = base::bits::CountPopulation(mask);
uint64_t mask_trailing_zeros = base::bits::CountTrailingZeros64(mask);
if ((mask_width != 0) && (mask_width != 64) &&
(base::bits::CountLeadingZeros64(mask) + mask_width +
mask_trailing_zeros ==
64)) {
const ShiftOp& shift = lhs.Cast<ShiftOp>();
if (int64_t shift_by;
MatchSignedIntegralConstant(shift.right(), &shift_by) &&
static_cast<uint64_t>(shift_by & 0x3F) == mask_trailing_zeros) {
if (mask_width + mask_trailing_zeros >= 64) {
Emit(kArm64Lsl, g.DefineAsRegister(node), g.UseRegister(shift.left()),
g.UseImmediate64(static_cast<int32_t>(mask_trailing_zeros)));
} else {
// Select Ubfiz for And(Lsl(x, imm), mask) where the mask is
// contiguous and its trailing zeros match the shift.
Emit(kArm64Ubfiz, g.DefineAsRegister(node),
g.UseRegister(shift.left()),
g.TempImmediate(static_cast<int32_t>(mask_trailing_zeros)),
g.TempImmediate(static_cast<int32_t>(mask_width)));
}
return;
}
}
}Can we extract this to a helper and reuse it for both VisitWord64And and VisitWord32And? (if the result looks too ugly because of the small differences between the 2, feel free not to do it)
Also here, the helper turn out to be comparable in size given the differences between the two. I would stick also here to keep it separate.
Add a comment here as well.
Done
I'd merge this into the `if` right above
- Darius Mercadier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
if ((mask_width != 0) && (mask_width != 32) &&
(base::bits::CountLeadingZeros32(mask) + mask_width +
mask_trailing_zeros ==
32)) {Marco VitaleOk so if I understand correctly, this matches a integer that just has a "pack" of 1s, like `0b00001111100000`. Let's extract this to a helper, add a comment explaining what it does, and reuse if also in VisitWord64And.
Unfortunately the helper is overly verbose, and it needs to return width, msb, and trailing_zeros (or taking them by ref, that it is something that i usually don't like). I would stick to the current version, as is consistent, like line 2125. Wdyt?
- Marco Vitale
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marco Vitale (Gerrit)
Marco Vitale added 1 comment![]( "Open in Gerrit")
if ((mask_width != 0) && (mask_width != 32) &&
(base::bits::CountLeadingZeros32(mask) + mask_width +
mask_trailing_zeros ==
32)) {Marco VitaleOk so if I understand correctly, this matches a integer that just has a "pack" of 1s, like `0b00001111100000`. Let's extract this to a helper, add a comment explaining what it does, and reuse if also in VisitWord64And.
Darius MercadierUnfortunately the helper is overly verbose, and it needs to return width, msb, and trailing_zeros (or taking them by ref, that it is something that i usually don't like). I would stick to the current version, as is consistent, like line 2125. Wdyt?
Wdyt of https://crrev.com/c/7726022 ?
My implementation was really similar, but still, we gain ~10 lines and we lose consistency with the rest of the file, for example, just above my code:
```if (int64_t constant_rhs;
lhs.Is<Opmask::kWord32ShiftRightLogical>() &&
CanCover(node, bitwise_and.left()) &&
MatchSignedIntegralConstant(bitwise_and.right(), &constant_rhs)) {
DCHECK(base::IsInRange(constant_rhs, std::numeric_limits<int32_t>::min(),
std::numeric_limits<int32_t>::max()));
uint32_t mask = static_cast<uint32_t>(constant_rhs);
uint32_t mask_width = base::bits::CountPopulation(mask);
uint32_t mask_msb = base::bits::CountLeadingZeros32(mask);
if ((mask_width != 0) && (mask_width != 32) &&
(mask_msb + mask_width == 32)) {
...
```
At least, this is my opinion, if you strongly prefer the helper we can proceed to your solution 😊Related details
- Darius Mercadier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
In my opinion, the goal is not to reduce code size, but rather to avoid duplicating the same optimizations. What happens the day we realize that there is a bug in the 32-bit version and whoever fixes it doesn't realize that this also needs to be fixed in the 64-bit version?
Having duplicated code is not a feature of this file, it's an unfortunate situation. If the optimization above your code can easily be extracted to a helper, then we should do so (not necessarily you, and definitely not in this CL of course).
Let's go with the approach that doesn't duplicate code. Feel free to reuse my code or use your own ;-)
Related details
- Marco Vitale
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marco Vitale (Gerrit)
Marco Vitale added 1 comment![]( "Open in Gerrit")
I'm always a bit reluctant when the usages are just two, and usually we pay a price in clarity with the helper (given the machinery needed to get the helper works). Also the TryEmit pattern is not helping IMO, as the duplicate logic is in the `Try` phase and in the and we are emitting different instructions. I also spent a bit of time to try to see if at least the emission could have be split, but it is still not great. Let's go with your solution! Maybe in the future, when i'll be more used to the code i can think about something better, and will ask you again for review. Thanks for the discussion!
Related details
- Darius Mercadier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 6 comments![]( "Open in Gerrit")
// Deduce the correct unsigned integer type based on bit length.Not a particularly useful comment, imo.
// Base::bits relies on C++ function overloading, so passing the deduced
// 'Int' automatically routes to the 32-bit or 64-bit implementations.Not convinced that this is a useful comment. It's also the case of `MatchUnsignedIntegralConstant` above btw.
But feel free to keep it if you think it's useful.
if ((mask_width != 0) && (mask_width != 32) &&
(base::bits::CountLeadingZeros32(mask) + mask_width +
mask_trailing_zeros ==
32)) {In that case, I think that we gained in clarity by using the helper: we now have `bit_length` and `kBitLengthMask` instead of magic numbers `64` (ok this one is easy to understand) and `0x3f` (this one on the other hand, I can't tell by just looking at it that it's actually 63, and with a named variable its meaning is also clearer).
Yes, those things could have been fixed without the helper, but overall in that specific case, the helper doesn't feel harder to read to me.
g.TempImmediate(static_cast<int32_t>(mask_trailing_zeros)),Any reason for using TempImmediate instead of UseImmediate?
(looks to me like TempImmediate should actually be globally remove and UseImmediate always used instead, but maybe I'm missing something)
g.TempImmediate(static_cast<int32_t>(mask_width)));same
if (uint64_t shift_by;
lhs.Is<Opmask::kWord64BitwiseAnd>() && CanCover(node, shift_op.left()) &&
MatchUnsignedIntegralConstant(shift_op.right(), &shift_by) &&
base::IsInRange(shift_by, 1, 63)) {
const WordBinopOp& bitwise_and = lhs.Cast<WordBinopOp>();
if (uint64_t mask;
MatchUnsignedIntegralConstant(bitwise_and.right(), &mask)) {
uint64_t mask_width = base::bits::CountPopulation(mask);
uint64_t mask_msb = base::bits::CountLeadingZeros64(mask);
if ((mask_width != 0) && (mask_msb + mask_width == 64)) {
// The mask must be contiguous and starting from the least-significant
// bit.
DCHECK_EQ(0u, base::bits::CountTrailingZeros64(mask));
if ((shift_by + mask_width) >= 64) {
// If the mask is contiguous and reaches or extends beyond the top
// bit, only the shift is needed.
Emit(kArm64Lsl, g.DefineAsRegister(node),
g.UseRegister(bitwise_and.left()), g.UseImmediate64(shift_by));
return;
} else {
// Select Ubfiz for Shl(And(x, mask), imm) where the mask is
// contiguous, and the shift immediate non-zero.
Emit(kArm64Ubfiz, g.DefineAsRegister(node),
g.UseRegister(bitwise_and.left()), g.UseImmediate64(shift_by),
g.TempImmediate(static_cast<int32_t>(mask_width)));
return;
}
}
}
}I'd also move this to a helper (as far as I can see this is basically the same code as in Word32Shl).
Related details
- Marco Vitale
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marco Vitale (Gerrit)
Marco Vitale added 4 comments![]( "Open in Gerrit")
// Deduce the correct unsigned integer type based on bit length.Not a particularly useful comment, imo.
Done
g.TempImmediate(static_cast<int32_t>(mask_trailing_zeros)),Any reason for using TempImmediate instead of UseImmediate?
(looks to me like TempImmediate should actually be globally remove and UseImmediate always used instead, but maybe I'm missing something)
Done
g.TempImmediate(static_cast<int32_t>(mask_width)));Marco Vitalesame
Done
if (uint64_t shift_by;I'd also move this to a helper (as far as I can see this is basically the same code as in Word32Shl).
- Darius Mercadier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |