Ensure side effects have been accounted for before swapping operands

Author: tannergooding

src/coreclr/jit/gentree.cpp

@@ -19875,7 +19875,11 @@ GenTree* Compiler::gtNewSimdBinOpNode(
             }
 
             // GT_AND_NOT expects `op1 & ~op2`, but xarch does `~op1 & op2`
+            // We expect op1 to have already been spilled
+
+            assert((op1->gtFlags & GTF_SIDE_EFFECT) == 0);
             std::swap(op1, op2);
+
             break;
         }
 
@@ -24234,58 +24238,76 @@ GenTree* Compiler::gtNewSimdTernaryLogicNode(var_types   type,
             {
                 case TernaryLogicUseFlags::A:
                 {
-                    // One operand. We swap it with C so it can be contained where possible
-                    value1 = op1;
-                    value2 = nullptr;
+                    // We'll swap from 'A, B, C' to 'B, C, A'
+                    // We expect A to have been spilled
 
-                    std::swap(op1, op3);
+                    assert((op1->gtFlags & GTF_SIDE_EFFECT) == 0);
+
+                    std::swap(op1, op2); // B, A, C
+                    std::swap(op2, op3); // B, C, A
+
+                    value1 = op3;
+                    value2 = nullptr;
                     break;
                 }
 
                 case TernaryLogicUseFlags::B:
                 {
-                    // One operand. We swap it with C so it can be contained where possible
-                    value1 = op2;
-                    value2 = nullptr;
+                    // We'll swap from 'A, B, C' to 'A, C, B'
+                    // We expect A and B to have been spilled
+
+                    assert((op1->gtFlags & GTF_SIDE_EFFECT) == 0);
+                    assert((op2->gtFlags & GTF_SIDE_EFFECT) == 0);
+
+                    std::swap(op2, op3); // A, C, B
 
-                    std::swap(op2, op3);
+                    value1 = op3;
+                    value2 = nullptr;
                     break;
                 }
 
                 case TernaryLogicUseFlags::C:
                 {
-                    // One operand. It is already in the ideal position
+                    // We don't require any operands to have been spilled
+
                     value1 = op3;
                     value2 = nullptr;
                     break;
                 }
 
                 case TernaryLogicUseFlags::AB:
                 {
-                    // Two operands. We swap A with C so it can be contained where possible
-                    // and so we aren't RMW
+                    // We'll swap from 'A, B, C' to 'C, A, B'
+                    // We expect A and B to have been spilled
 
-                    value1 = op1;
-                    value2 = op2;
+                    assert((op1->gtFlags & GTF_SIDE_EFFECT) == 0);
+                    assert((op2->gtFlags & GTF_SIDE_EFFECT) == 0);
 
-                    std::swap(op1, op3);
+                    std::swap(op1, op3); // C, B, A
+                    std::swap(op2, op3); // C, A, B
+
+                    value1 = op2;
+                    value2 = op3;
                     break;
                 }
 
                 case TernaryLogicUseFlags::AC:
                 {
-                    // Two operands. We swap A with B so we aren't RMW
+                    // We'll swap from 'A, B, C' to 'B, C, A'
+                    // We expect A to have been spilled
 
-                    value1 = op1;
-                    value2 = op3;
+                    assert((op1->gtFlags & GTF_SIDE_EFFECT) == 0);
+
+                    std::swap(op1, op2); // B, A, C
 
-                    std::swap(op1, op2);
+                    value1 = op2;
+                    value2 = op3;
                     break;
                 }
 
                 case TernaryLogicUseFlags::BC:
                 {
-                    // Two operands. They are already in the ideal positions
+                    // We don't require any operands to have been spilled
 
                     value1 = op2;
                     value2 = op3;
@@ -24469,6 +24491,9 @@ GenTree* Compiler::gtNewSimdTernaryLogicNode(var_types   type,
                             value1 = gtNewOperNode(GT_COMMA, type, op1, value1);
                         }
 
+                        // GT_AND_NOT takes them as `op1 & ~op2` and x86 reorders them back to `~op1 & op2`
+                        // since the underlying andnps/andnpd/pandn instructions take them as such
+
                         return gtNewSimdBinOpNode(GT_AND_NOT, type, value2, value1, simdBaseJitType, simdSize);
                     }
                     else

src/coreclr/jit/hwintrinsicxarch.cpp

@@ -3002,6 +3002,96 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic        intrinsic,
             assert(sig->numArgs == 4);
 
             op4 = impPopStack().val;
+
+            bool spillOp1 = false;
+            bool spillOp2 = false;
+
+            if (op4->IsIntegralConst())
+            {
+                uint8_t                 control  = static_cast<uint8_t>(op4->AsIntCon()->gtIconVal);
+                const TernaryLogicInfo& info     = TernaryLogicInfo::lookup(control);
+                TernaryLogicUseFlags    useFlags = info.GetAllUseFlags();
+
+                if (useFlags != TernaryLogicUseFlags::ABC)
+                {
+                    switch (useFlags)
+                    {
+                        case TernaryLogicUseFlags::A:
+                        {
+                            // We'll swap from 'A, B, C' to 'B, C, A'
+                            // so just spill A
+
+                            spillOp1 = true;
+                            break;
+                        }
+
+                        case TernaryLogicUseFlags::B:
+                        {
+                            // We'll swap from 'A, B, C' to 'A, C, B'
+                            // so spill A and B
+
+                            spillOp1 = true;
+                            spillOp2 = true;
+                            break;
+                        }
+
+                        case TernaryLogicUseFlags::C:
+                        {
+                            // No operands will be swapped
+                            break;
+                        }
+
+                        case TernaryLogicUseFlags::AB:
+                        {
+                            // We'll swap from 'A, B, C' to 'C, A, B'
+                            // so spill A and B
+
+                            spillOp1 = true;
+                            spillOp2 = true;
+                            break;
+                        }
+
+                        case TernaryLogicUseFlags::AC:
+                        {
+                            // We'll swap from 'A, B, C' to 'B, C, A'
+                            // so just spill A
+
+                            spillOp1 = true;
+                            break;
+                        }
+
+                        case TernaryLogicUseFlags::BC:
+                        {
+                            // No operands will be swapped
+                            break;
+                        }
+
+                        case TernaryLogicUseFlags::None:
+                        {
+                            // No operands.
+                            break;
+                        }
+
+                        default:
+                        {
+                            unreached();
+                        }
+                    }
+                }
+            }
+
+            if (spillOp1)
+            {
+                impSpillSideEffect(true, verCurrentState.esStackDepth -
+                                             3 DEBUGARG("Spilling op1 side effects for HWIntrinsic"));
+            }
+
+            if (spillOp2)
+            {
+                impSpillSideEffect(true, verCurrentState.esStackDepth -
+                                             2 DEBUGARG("Spilling op2 side effects for HWIntrinsic"));
+            }
+
             op3 = impSIMDPopStack();
             op2 = impSIMDPopStack();
             op1 = impSIMDPopStack();