Make sure to remove operand of TernaryLogic node

[kunalspathak]

Fixes: #116568

[Copilot]

Pull Request Overview

This PR addresses the issue of removing the operand of a TernaryLogic node during hardware intrinsic lowering. It introduces a new test project and source file to verify the fix and updates the lowering code in lowerxarch.cpp to properly remove the operand when a condition is present.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/tests/JIT/Regression/JitBlue/Runtime_116568/Runtime_116568.csproj	New csproj file for the test project
src/tests/JIT/Regression/JitBlue/Runtime_116568/Runtime_116568.cs	Added test code verifying TernaryLogic behavior
src/coreclr/jit/lowerxarch.cpp	Updated lowering logic for TernaryLogic node to remove the operand

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[kunalspathak]

@dotnet/jit-contrib

[tannergooding]

Is this going to conflict with the node->ResetHWIntrinsicId below?

Should it rather be conditioned on the if (!condition->OperIsHWIntrinsic()) which causes it to break?

[tannergooding]

I think it is actually. I'm pretty sure the issue here is we want this instead...

if (tmp->OperIsHWIntrinsic())
{
    BlockRange().Remove(condition);
    condition = tmp;
}

We should always find a user for condition, because it's always going to be op1, op2, or op3 of node.

So by replacing the use in the case tmp isn't a HWIntrinsic, we're going to end up with TernaryLogic(mask) which will result in bad codegen.

We instead only want to remove condition in the scenario we know that tmp will be used in the ResetHWIntrinsic call, so it needs to be conditioned as per the above.

---

This all stems from us needing to know how many bits are in the mask so that the BlendVariable is configured with the right type so the behavior stays the same.

[kunalspathak]

We are operating on t92 and the condition node is t85. In main today, we remove it, but then leave the t235 node untouched leading to the failure.

LowerHWIntrinsicTernaryLogic: (before)
N001 (  3,  2) [000083] -----------                   t83 =    LCL_VAR   simd32<System.Runtime.Intrinsics.Vector256`1> V02 loc2         u:2 $100
N002 (  3,  2) [000084] -----------                   t84 =    LCL_VAR   simd32<System.Runtime.Intrinsics.Vector256`1> V01 loc1         u:2 $101
N003 (  3,  2) [000087] -----------                   t87 =    LCL_VAR   simd32<System.Runtime.Intrinsics.Vector256`1> V00 loc0         u:2 $100
N004 (  3,  2) [000088] -----------                   t88 =    LCL_VAR   simd32<System.Runtime.Intrinsics.Vector256`1> V04 loc4         u:2 <l:$340, c:$341>
                                                            ┌──▌  t87    simd32 
                                                            ├──▌  t88    simd32 
N005 (  7,  5) [000086] -----------                   t86 = ▌  HWINTRINSIC mask   32 ushort CompareGreaterThanOrEqualMask <l:$380, c:$381>
                                                            ┌──▌  t86    mask   
N006 ( 11,  8) [000234] DA---------                         ▌  STORE_LCL_VAR mask   V11 cse4         d:1 $VN.Void
N007 (  3,  2) [000235] -----------                  t235 =    LCL_VAR   mask   V11 cse4         u:1 <l:$380, c:$381>
                                                            ┌──▌  t235   mask   
N009 ( 15, 11) [000085] -A---------                   t85 = ▌  HWINTRINSIC simd32 32 ushort ConvertMaskToVector $84
N010 (  1,  4) [000089] -------N---                   t89 =    CNS_INT   int    216 $47
                                                            ┌──▌  t83    simd32 
                                                            ├──▌  t84    simd32 
                                                            ├──▌  t85    simd32 
                                                            ├──▌  t89    int    
N011 ( 23, 20) [000082] -A---------                   t82 = ▌  HWINTRINSIC simd32 32 uint TernaryLogic $85

With my change, here is what we do after updating. Is that not accurate?

LowerHWIntrinsicTernaryLogic: (after)
N001 (  3,  2) [000083] -----------                   t83 =    LCL_VAR   simd32<System.Runtime.Intrinsics.Vector256`1> V02 loc2         u:2 $100
N002 (  3,  2) [000084] -----------                   t84 =    LCL_VAR   simd32<System.Runtime.Intrinsics.Vector256`1> V01 loc1         u:2 $101
N003 (  3,  2) [000087] -----------                   t87 =    LCL_VAR   simd32<System.Runtime.Intrinsics.Vector256`1> V00 loc0         u:2 $100
N004 (  3,  2) [000088] -----------                   t88 =    LCL_VAR   simd32<System.Runtime.Intrinsics.Vector256`1> V04 loc4         u:2 <l:$340, c:$341>
                                                            ┌──▌  t87    simd32 
                                                            ├──▌  t88    simd32 
N005 (  7,  5) [000086] -----------                   t86 = ▌  HWINTRINSIC mask   32 ushort CompareGreaterThanOrEqualMask <l:$380, c:$381>
                                                            ┌──▌  t86    mask   
N006 ( 11,  8) [000234] DA---------                         ▌  STORE_LCL_VAR mask   V11 cse4         d:1 $VN.Void
N007 (  3,  2) [000235] -----------                  t235 =    LCL_VAR   mask   V11 cse4         u:1 <l:$380, c:$381>
N010 (  1,  4) [000089] -------N---                   t89 =    CNS_INT   int    216 $47
                                                            ┌──▌  t83    simd32 
                                                            ├──▌  t84    simd32 
                                                            ├──▌  t235   mask   
                                                            ├──▌  t89    int    
N011 ( 23, 20) [000082] -A---------                   t82 = ▌  HWINTRINSIC simd32 32 uint TernaryLogic $85
```

If we add the condition of `tmp->OperIsHWIntrinsic()`, `condition` stays `ConvertMaskToVector()` and we hit assert `                assert(varTypeIsMask(condition));` few lines below.

[tannergooding]

With my change, here is what we do after updating. Is that not accurate?

No, because you can see that TernaryLogic is taking t235 directly. This will lead to incorrect codegen because TernaryLogic expects a simd32 here:

                                                            ┌──▌  t83    simd32 
                                                            ├──▌  t84    simd32 
                                                            ├──▌  t235   mask   
                                                            ├──▌  t89    int    
N011 ( 23, 20) [000082] -A---------                   t82 = ▌  HWINTRINSIC simd32 32 uint TernaryLogic $85

We rather shouldn't be removing condition (t85) at all, because t235 is a LCL_VAR rather than a HWINTRINSIC node, and we don't currently have a way of knowing how many mask bits are in that LCL_VAR node.

[tannergooding]

So, by instead changing it to the following we keep things correct and avoid the issue altogether:

if (tmp->OperIsHWIntrinsic())
{
    BlockRange().Remove(condition);
    condition = tmp;
}
else
{
    // We can't change to a BlendVariable intrinsic, so bail out
    break;
}

[tannergooding]

Believe a tweak is required after thinking it through some more.

[kunalspathak]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).