Skip to content

Use LSB of vector when converting from vector to mask #116991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Use LSB of vector when converting from vector to mask #116991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/coreclr/jit/hwintrinsiccodegenarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2010,10 +2010,19 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
break;

case NI_Sve_ConvertVectorToMask:
{
// PMOV would be ideal here, but it is in SVE2.1.
// Instead, use a compare: CMPNE <Pd>.<T>, <Pg>/Z, <Zn>.<T>, #0
GetEmitter()->emitIns_R_R_R_I(ins, emitSize, targetReg, op1Reg, op2Reg, 0, opt);
Comment on lines 2014 to -2015
Copy link
Member

@tannergooding tannergooding Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure this is correct and will likely break real world code.

The general consideration is that what we expose to the end user is a "full vector", so if they do something like Vector.ConditionalSelect(mask, x, y) then they are expecting this to behave exactly as (x & mask) | (y & ~mask), which is that it selects x when the corresponding bit in mask is set and y when it is clear.

The user isn't thinking about or considering the fact that on xarch the blendv instruction only checks the msb or any other similar nuance. They are writing a general purpose cross-platform API where the contract is "bitwise" from the original vector.

Correspondingly, we've documented that APIs which take a mask input are expecting allbitsset for that element to indicate true and zero for that element to indicate false. There isn't a consideration for them that the underlying hardware happens to have a conversion to/from the internal mask/predicate register representation that can optimize it a particular way.

The reason we document this is because all the hardware that doesn't have a dedicated masking register concept has their comparisons return AllBitsSet or Zero per element, because its the most effective thing to use when dealing purely with vector data and it makes this experience consistent regardless of where you are or what you're doing.

Now, there are some dedicated instructions like Sse41.BlendVariable which explicitly take a vector in hardware and which explicitly document that they only consider the MSB of each element, but these are special and require direct usage. They aren't something we always transform something like Vector128.ConditionalSelect into, because that would result in a different semantic for unknown vectors (anything that wasn't AllBitsSet vs Zero per element).

I believe the same consideration exists here. We cannot implicitly have the internal ConvertVectorToMask helper behaving as if it is (x & 1) != 0, because it will break the rest of the logic flow and normalization considerations we've taken throughout the JIT and laid down for users.

More broadly, I'm not understanding what is meant by "the predicate registers conceptually only represents a bit (LSB) of the corresponding vector lanes"

The manual simply documents

Each SVE predicate register holds one bit for each byte of an SVE scalar vector register.

It then covers:

A predicate-as-mask encoding is divided into 1-bit, 2-bit, 4-bit, or 8-bit predicate elements. Each predicate element corresponds to a vector element in an SVE scalable vector register
...
If the lowest-numbered bit of a predicate element is 0, the value of the predicate element is FALSE.
If the lowest-numbered bit of a predicate element is 1, the value of the predicate element is TRUE.

Which is to say that for Vector128<byte> you get 16x 1-bit predicate values. For Vector128<ushort> you get 8x 2-bit predicate values, V128<uint> is 4x 4-bit, and V128<ulong> is 2x 8-bit.

For the multi-bit predicates, you get the consideration that only the lsb of the predicate element is actually checked. That is, for V128<ushort> you get the following, showing that only the lsb is relevant:

  • 00 - false
  • 01 - true
  • 10 - false
  • 11 - true

Which would indicate that the conversion being done here isn't really "correct" in the first place. The existing logic was already producing a correct predicate, but more so a more useful predicate because it can freely be reused for any smaller T. That is, because we only convert to 00 or 11 predicates, the V128<short> is equally reusable as a predicate for V128<byte> since it remains a valid bytewise predicate for the same comparison. If you normalized this to only 00 vs 01, we'd need to then normalize that for use with V128<byte> (and so on for 4-bit and 8-bit predicate elements).

Given that, I'd think any "incorrect" behavior would potentially be in ConvertMaskToVector where something could've produced a Mask for ushort elements that was 00/01/10/11 and where we'd want to produce a Vector that was still Zero/AllBitsSet/Zero/AllBitsSet for those respective values; but I believe the conversion codegen is already handling that nuance due to how it uses the mask as part of the selection.

Copy link
Member

@tannergooding tannergooding Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I don't think we have any consideration where a managed API is taking a Vector<T> where we aren't expecting it to be precisely Zero vs Non-Zero for the mask elements for conversion purposes and where we aren't requiring that a mask producing node normalize to Zero vs AllBitsSet per element.

We only have considerations for what internal nodes might produce when directly consumed, but those are never producing vectors where only the LSB is considered as far as I can tell. There is only the case that a mask might only set the lsb of the predicate element and where we should already be handling that nuance.

Copy link
Contributor

@a74nh a74nh Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider an XOR where the result is then used as a mask:

ConvertVectorToMask(Xor(veca, vecb)):
veca = {100, 111, 101, 000}
vecb = {100, 100, 101, 000}
xor =  {000, 011, 000, 000}
to mask = {0, 1, 0, 0}

By the rules we've set up, this should be exactly the same as first converting the inputs to a mask, and then XORing using the predicate XOR instruction. But today this gives a different result:

XorPredicate(ConvertVectorToMask(veca), ConvertVectorToMask(vecb)):
veca = {100, 111, 101, 000}
vecb = {100, 100, 101, 000}
maska = {1, 1, 1, 0}
maskb = {1, 1, 1, 0}
xor = {0, 0, 0, 0}

Using the LSB for masks and now the results match:

ConvertVectorToMask(Xor(veca, vecb)):
veca = {100, 111, 101, 000}
vecb = {100, 100, 101, 000}
xor  = {000, 011, 000, 000}
tomask = {0, 1, 0, 0}

XorPredicate(ConvertVectorToMask(veca), ConvertVectorToMask(vecb)):
veca = {100, 111, 101, 000}
vecb = {100, 100, 101, 000}
maska = {0, 1, 1, 0}
maskb = {0, 0, 1, 0}
xor {0, 1, 0, 0}

In simd.h the code was using MSB when converting from Vector to Mask. As of my #115566, it's now using LSB. Either way, neither of these options match the idea of using all true for a mask.

Previously we got away with this because we kept masks as TrueVector nodes and so none of the constant folding optimisations ever got triggered. As of #115566 those optimisations are getting triggered by Fuzzlyn when passing all constant vectors into ConditionalSelect and other such oddities.

Copy link
Member

@tannergooding tannergooding Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the rules we've set up, this should be exactly the same

This shouldn't be part of the existing rules/logic. Is there a place that is doing this in the Arm64 code already? If so, I think that's a bug and what needs to be fixed.

This is the same general issue of bool ^ bool where C# defines it as 0 (literal false) vs 1 (literal true) where-as IL defines it as 0 (literal false) vs not-0 (conceptual true) and so simply doing (x ^ y) == true can be distinct from (x ^ y) != false, because doing a literal xor of booleans can produce something that isn't the same as the literal true.

For anything like Sve.CompareEqual we know that it produces a predicate result. This will actually only set the lsb of the respective "predicate element". However, we guarantee that to the end user if they end up viewing it as a vector it will only be AllBitsSet (predicate was true for that element) or Zero (predicate was false for that element) and so the CvtMaskToVector will do the right thing for that platform (on both xarch and arm64 this is that each element has 1 bit to check per element, the difference is how those bits are packed throughout the predicate register).

The user then doesn't have an API that lets them directly do XorPredicate, we only light that up via internal pattern recognition and we know the base type of a given mask, so we know whether two marks are compatible. This is correspondingly why the morph logic that produces XorMask requires it to explicitly be in the shape of Xor(CvtMaskToVector(mask), CvtMaskToVector(mask)) and requires the two masks to be for compatible SIMD sizes/base types, because otherwise it could produce a difference in behavior. -- In general it isn't safe to do predicate1Bit ^ predicate2Bit because they aren't logically compatible. There are some special scenarios involving constants where you can treat them as compatible due to being able to observe the bit patterns, but otherwise you have 1-bit per byte vs 1-bit per 2 bytes, so you'd get a potentially incorrect result whether it was consumed as 1-bit predicate element or 2-bit predicate element value.

Copy link
Contributor

@a74nh a74nh Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user then doesn't have an API that lets them directly do XorPredicate, we only light that up via internal pattern recognition and we know the base type of a given mask

As I understood it, the intention was that we would be able to optimise Xor to XorPredicate, otherwise we'd be missing SVE instructions. #101294 (comment) and other places. Recently #114438 added support for this, but this was then turned off with #115566. #116854 attempts to enable it again, but it's stuck behind this PR.

This is correspondingly why the morph logic that produces XorMask requires it to explicitly be in the shape of Xor(CvtMaskToVector(mask), CvtMaskToVector(mask)) and requires the two masks to be for compatible SIMD sizes/base types,

Right. It should be fairly simple to update #116854 to work like that.

There are also the approved SVE APIs for AndNot and OrNot. The instruction for these are predicates only. Using allbit masks I'm not sure if we can safely implement them or not. #116628 adds those APIs, but is stuck behind all the above work.

How should a constant vector be turned into a mask if the user passes it into an API that uses a mask? EvaluateSimdCvtVectorToMask() in simd.h used to check MSB, and now checks LSB. What bit or bits should be turned into true? Only allbitsset?

Given we're getting into July now, I'm quite keen to get this all fixed up ASAP.

Copy link
Member

@tannergooding tannergooding Jun 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understood it, the intention was that we would be able to optimise Xor to XorPredicate

Yes, but only where the optimization is valid. We cannot do it when it would change semantics to managed user.

In practice this hasn't been an issue for any existing code paths. Developers are not taking a mask for Vector<byte> and combining it with a mask for Vector<int> in the wild. -- They notably wouldn't have been able to combine them properly using raw predicates either, just due to the two masks being fundamentally incompatible due to representing different state.

Right. It should be fairly simple to update #116854 to work like that.

This seems like a fairly expensive/verbose approach. The xarch logic is far simpler, https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/morph.cpp#L10009-L10147, and it catches all the important scenarios for very little expense.

The existing logic just looks for BitwiseOp(CvtMaskToVec(op1), CvtMaskToVec(op2)) and transforms it to BitwiseOpMask(op1, op2) if the two inputs are compatible. This takes a handful of lines of code and reasonable Arm64 should be able to light up with the same logic, just by adding the relevant maskIntrinsicId selections for Arm64 SVE. -- It and other paths that are handling the x64 KMASK setup were intentionally setup so that Arm64 Predicates could be plugged in trivially when ready.

There are also the approved SVE APIs for AndNot and OrNot. The instruction for these are predicates only. Using allbit masks I'm not sure if we can safely implement them or not

If they are only for predicates, they likely shouldn't be exposed. There's not really a need to expose them either and I think the names are potentially confusing due to the them being nand/nor -- We already have AndNot APIs and it is expected they behave like BIC does, which is x & ~y, where-as the predicates are doing ~(x & y) and ~(x | y) instead.

x64 has a similar predicate only concept with xnor which does ~(x ^ y), but it not having a managed API isn't problematic because the user is more likely to write the natural pattern (which can also be optimized for other platforms or ISAs, like AdvSimd). We simply recognize NOT(XOR(CvtMaskToVec(x), CvtMaskToVec(y))) and translate it to XnorMask(x, y) -- The normalization to NOT(XOR(...)) also makes it easier to light up constant folding, comparison inversions, and other basic optimizations; so we intentionally decompose things like V128.AndNot(x, y) into AND(x, NOT(y)) instead of importing it directly as AND_NOT(x, y). We then construct the actual AND_NOT much later, such as in lowering, when we know that no other optimization work is needed and we just want to optimize the instruction selection

How should a constant vector be turned into a mask if the user passes it into an API that uses a mask?

We basically have 4 options here:

  • non-zero is true, zero is false
    • This most closely corresponds to how bool works
  • all-bits-set is true, anything else is false
  • msb set is true, msb clear is false
    • This most closely corresponds to how vpmov*2m works on xarch
  • lsb set is true, lsb clear is false
    • This most closely corresponds to how pmov (to predicate) works on arm64

The intended behavior is the first: non-zero is true, zero is false. This is the intended behavior because it corresponds to how bool works already, is the cheapest to emulate for unknown inputs, lets us trivially optimize for both xarch and arm64 for known inputs, and allows cross platform determinism (including simplistic mapping to hardware without specialized predicate/masking support).

In practice the need to use this conversion should be rare, because users should not be taking arbitrary (especially non-constant) vectors and trying to use them as inputs into scenarios that expect predicates/masks. Most typically code is using the output of some Compare or similar instruction which directly produced a mask, potentially doing some minor bitwise operations on it to combine it with other masks.

Copy link
Contributor

@a74nh a74nh Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice the need to use this conversion should be rare, because users should not be taking arbitrary (especially non-constant) vectors and trying to use them as inputs into scenarios that expect predicates/masks.

Agreed, but sadly Fuzzlyn doesn't agree - currently this is where all the failures are coming from, with code I doubt we'd ever see in the wild.

I'll get this fixed up this week. I'll draft a new PR to fix the mask handling. And then when combined with #116852 that should fix all the current issues.

Copy link
Member

@tannergooding tannergooding Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently this is where all the failures are coming from, with code I doubt we'd ever see in the wild.

Right. It's important we still handle it correctly and it's great Fuzzlyn is catching them. Just it's not critical for it to be "peak efficiency" for codegen since it's not likely going to happen in the real world.

Rather we want to ensure that morph/folding/lowering catch the typical real world cases and get the good codegen there. From what I've seen of the existing x64 support (which has been in production a couple years now), we're getting most of the right stuff already and most of that is setup for Arm64 to hook in for the same benefits.

Copy link
Contributor

@a74nh a74nh Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put the required changes to EvaluateSimdCvtVectorToMask() in #116852.

This PR should probably be closed now.

//
// Instead, to test if lowest bit is set, we LSL elementWidthInBits - 1
// and then compare: CMPNE <Pd>.<T>, <Pg>/Z, <Zn>.<T>, #0

int elementWidthInBits = 8 * genTypeSize(intrin.baseType);
regNumber ztemp = internalRegisters.GetSingle(node);

GetEmitter()->emitIns_R_R_I(INS_sve_lsl, emitSize, ztemp, op2Reg, elementWidthInBits - 1, opt);
GetEmitter()->emitIns_R_R_R_I(ins, emitSize, targetReg, op1Reg, ztemp, 0, opt);
break;
}

case NI_Sve_Count16BitElements:
case NI_Sve_Count32BitElements:
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/jit/lsraarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1400,6 +1400,12 @@ int LinearScan::BuildHWIntrinsic(GenTreeHWIntrinsic* intrinsicTree, int* pDstCou
delayFreeOp = getDelayFreeOperand(embeddedOp, /* embedded */ true);
}

if (intrin.id == NI_Sve_ConvertVectorToMask)
{
// Need an extra temp to test LSB of source vector
buildInternalFloatRegisterDefForNode(intrinsicTree);
}

// Build any immediates
BuildHWIntrinsicImmediate(intrinsicTree, intrin);

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/optimizemaskconversions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ struct MaskConversionsWeight
static constexpr const weight_t costOfConvertMaskToVector = 1.0;

#if defined(TARGET_ARM64)
// Conversion of vector to mask is two instructions.
static constexpr const weight_t costOfConvertVectorToMask = 2.0;
// Conversion of vector to mask is three instructions.
static constexpr const weight_t costOfConvertVectorToMask = 3.0;
#else
// Conversion of vector to mask is one instructions.
static constexpr const weight_t costOfConvertVectorToMask = 1.0;
Expand Down
Loading