Fix for bug 12398: Lowering is inconsistent in checking safety of RegOptional....

Fix for bug 12398: Lowering is inconsistent in checking safety of RegOptional. (dotnet/coreclr#19740)

This fixes an inconsistency in lowering where it fails to make an
operand contained because IsSafeToContainMem() returns false yet
it marks it regOptional, which may cause a problem if the operand
will be loaded at the point of use.

I also fixed a case where an operand was marked RegOptional even though
there is a type size mismatch.

There are 7 places that are affected, I added repro cases for 4 of them.
I wasn't able to construct repros for the 3 places that deal with
floating point operands but decided to fix those places anyway.

Fixes dotnet/coreclr#12398.

Commit migrated from https://github.com/dotnet/coreclr/commit/36449cf94237e7cddb7e43a9fe6f873e7f5ebf77

src/coreclr/src/jit/lower.cpp

@@ -4874,7 +4874,6 @@ GenTree* Lowering::LowerConstIntDivOrMod(GenTree* node)
 
         newDivMod = comp->gtNewOperNode(GT_SUB, type, comp->gtNewLclvNode(dividendLclNum, type),
                                         comp->gtNewOperNode(GT_AND, type, adjustedDividend, divisor));
-        ContainCheckBinary(newDivMod->AsOp());
     }
 
     // Remove the divisor and dividend nodes from the linear order,

src/coreclr/src/jit/lower.h

@@ -229,15 +229,18 @@ private:
     // operands.
     //
     // Arguments:
-    //     tree  -  Gentree of a binary operation.
+    //     tree  -             Gentree of a binary operation.
+    //     isSafeToMarkOp1     True if it's safe to mark op1 as register optional
+    //     isSafeToMarkOp2     True if it's safe to mark op2 as register optional
     //
     // Returns
-    //     None.
+    //     The caller is expected to get isSafeToMarkOp1 and isSafeToMarkOp2
+    //     by calling IsSafeToContainMem.
     //
     // Note: On xarch at most only one of the operands will be marked as
     // reg optional, even when both operands could be considered register
     // optional.
-    void SetRegOptionalForBinOp(GenTree* tree)
+    void SetRegOptionalForBinOp(GenTree* tree, bool isSafeToMarkOp1, bool isSafeToMarkOp2)
     {
         assert(GenTree::OperIsBinary(tree->OperGet()));
 
@@ -246,8 +249,9 @@ private:
 
         const unsigned operatorSize = genTypeSize(tree->TypeGet());
 
-        const bool op1Legal = tree->OperIsCommutative() && (operatorSize == genTypeSize(op1->TypeGet()));
-        const bool op2Legal = operatorSize == genTypeSize(op2->TypeGet());
+        const bool op1Legal =
+            isSafeToMarkOp1 && tree->OperIsCommutative() && (operatorSize == genTypeSize(op1->TypeGet()));
+        const bool op2Legal = isSafeToMarkOp2 && (operatorSize == genTypeSize(op2->TypeGet()));
 
         GenTree* regOptionalOperand = nullptr;
         if (op1Legal)

src/coreclr/src/jit/lowerxarch.cpp

@@ -1553,25 +1553,54 @@ void Lowering::ContainCheckMul(GenTreeOp* node)
     GenTree* op1 = node->gtOp.gtOp1;
     GenTree* op2 = node->gtOp.gtOp2;
 
+    bool isSafeToContainOp1 = true;
+    bool isSafeToContainOp2 = true;
+
     // Case of float/double mul.
     if (varTypeIsFloating(node->TypeGet()))
     {
         assert(node->OperGet() == GT_MUL);
 
-        if (IsContainableMemoryOp(op2) || op2->IsCnsNonZeroFltOrDbl())
+        if (op2->IsCnsNonZeroFltOrDbl())
         {
             MakeSrcContained(node, op2);
         }
-        else if (op1->IsCnsNonZeroFltOrDbl() || (IsContainableMemoryOp(op1) && IsSafeToContainMem(node, op1)))
+        else if (IsContainableMemoryOp(op2))
+        {
+            isSafeToContainOp2 = IsSafeToContainMem(node, op2);
+            if (isSafeToContainOp2)
+            {
+                MakeSrcContained(node, op2);
+            }
+        }
+
+        if (!op2->isContained())
         {
             // Since  GT_MUL is commutative, we will try to re-order operands if it is safe to
             // generate more efficient code sequence for the case of GT_MUL(op1=memOp, op2=non-memOp)
-            MakeSrcContained(node, op1);
+            if (op1->IsCnsNonZeroFltOrDbl())
+            {
+                MakeSrcContained(node, op1);
+            }
+            else if (IsContainableMemoryOp(op1))
+            {
+                isSafeToContainOp1 = IsSafeToContainMem(node, op1);
+                if (isSafeToContainOp1)
+                {
+                    MakeSrcContained(node, op1);
+                }
+            }
         }
-        else
+
+        if (!op1->isContained() && !op2->isContained())
         {
             // If there are no containable operands, we can make an operand reg optional.
-            SetRegOptionalForBinOp(node);
+            // IsSafeToContainMem is expensive so we call it at most once for each operand
+            // in this method. If we already called IsSafeToContainMem, it must have returned false;
+            // otherwise, the corresponding operand (op1 or op2) would be contained.
+            isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1);
+            isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2);
+            SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2);
         }
         return;
     }
@@ -1639,21 +1668,42 @@ void Lowering::ContainCheckMul(GenTreeOp* node)
     //
     if (memOp == nullptr)
     {
-        if (IsContainableMemoryOp(op2) && (op2->TypeGet() == node->TypeGet()) && IsSafeToContainMem(node, op2))
+        if ((op2->TypeGet() == node->TypeGet()) && IsContainableMemoryOp(op2))
         {
-            memOp = op2;
+            isSafeToContainOp2 = IsSafeToContainMem(node, op2);
+            if (isSafeToContainOp2)
+            {
+                memOp = op2;
+            }
         }
-        else if (IsContainableMemoryOp(op1) && (op1->TypeGet() == node->TypeGet()) && IsSafeToContainMem(node, op1))
+
+        if ((memOp == nullptr) && (op1->TypeGet() == node->TypeGet()) && IsContainableMemoryOp(op1))
         {
-            memOp = op1;
+            isSafeToContainOp1 = IsSafeToContainMem(node, op1);
+            if (isSafeToContainOp1)
+            {
+                memOp = op1;
+            }
         }
     }
     else
     {
-        if ((memOp->TypeGet() != node->TypeGet()) || !IsSafeToContainMem(node, memOp))
+        if ((memOp->TypeGet() != node->TypeGet()))
         {
             memOp = nullptr;
         }
+        else if (!IsSafeToContainMem(node, memOp))
+        {
+            if (memOp == op1)
+            {
+                isSafeToContainOp1 = false;
+            }
+            else
+            {
+                isSafeToContainOp2 = false;
+            }
+            memOp = nullptr;
+        }
     }
     // To generate an LEA we need to force memOp into a register
     // so don't allow memOp to be 'contained'
@@ -1664,23 +1714,34 @@ void Lowering::ContainCheckMul(GenTreeOp* node)
         {
             MakeSrcContained(node, memOp);
         }
-        else if (imm != nullptr)
-        {
-            // Has a contained immediate operand.
-            // Only 'other' operand can be marked as reg optional.
-            assert(other != nullptr);
-            other->SetRegOptional();
-        }
-        else if (hasImpliedFirstOperand)
-        {
-            // Only op2 can be marke as reg optional.
-            op2->SetRegOptional();
-        }
         else
         {
-            // If there are no containable operands, we can make either of op1 or op2
-            // as reg optional.
-            SetRegOptionalForBinOp(node);
+            // IsSafeToContainMem is expensive so we call it at most once for each operand
+            // in this method. If we already called IsSafeToContainMem, it must have returned false;
+            // otherwise, memOp would be set to the corresponding operand (op1 or op2).
+            if (imm != nullptr)
+            {
+                // Has a contained immediate operand.
+                // Only 'other' operand can be marked as reg optional.
+                assert(other != nullptr);
+
+                isSafeToContainOp1 = ((other == op1) && isSafeToContainOp1 && IsSafeToContainMem(node, op1));
+                isSafeToContainOp2 = ((other == op2) && isSafeToContainOp2 && IsSafeToContainMem(node, op2));
+            }
+            else if (hasImpliedFirstOperand)
+            {
+                // Only op2 can be marked as reg optional.
+                isSafeToContainOp1 = false;
+                isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2);
+            }
+            else
+            {
+                // If there are no containable operands, we can make either of op1 or op2
+                // as reg optional.
+                isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1);
+                isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2);
+            }
+            SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2);
         }
     }
 }
@@ -1853,18 +1914,27 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
         }
 
         assert(otherOp != nullptr);
+        bool isSafeToContainOtherOp = true;
         if (otherOp->IsCnsNonZeroFltOrDbl())
         {
             MakeSrcContained(cmp, otherOp);
         }
-        else if (IsContainableMemoryOp(otherOp) && ((otherOp == op2) || IsSafeToContainMem(cmp, otherOp)))
+        else if (IsContainableMemoryOp(otherOp))
         {
-            MakeSrcContained(cmp, otherOp);
+            isSafeToContainOtherOp = IsSafeToContainMem(cmp, otherOp);
+            if (isSafeToContainOtherOp)
+            {
+                MakeSrcContained(cmp, otherOp);
+            }
         }
-        else
+
+        if (!otherOp->isContained() && isSafeToContainOtherOp && IsSafeToContainMem(cmp, otherOp))
         {
             // SSE2 allows only otherOp to be a memory-op. Since otherOp is not
             // contained, we can mark it reg-optional.
+            // IsSafeToContainMem is expensive so we call it at most once for otherOp.
+            // If we already called IsSafeToContainMem, it must have returned false;
+            // otherwise, otherOp would be contained.
             otherOp->SetRegOptional();
         }
 
@@ -1895,24 +1965,44 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
         // Note that TEST does not have a r,rm encoding like CMP has but we can still
         // contain the second operand because the emitter maps both r,rm and rm,r to
         // the same instruction code. This avoids the need to special case TEST here.
+
+        bool isSafeToContainOp1 = true;
+        bool isSafeToContainOp2 = true;
+
         if (IsContainableMemoryOp(op2))
         {
-            MakeSrcContained(cmp, op2);
-        }
-        else if (IsContainableMemoryOp(op1) && IsSafeToContainMem(cmp, op1))
-        {
-            MakeSrcContained(cmp, op1);
+            isSafeToContainOp2 = IsSafeToContainMem(cmp, op2);
+            if (isSafeToContainOp2)
+            {
+                MakeSrcContained(cmp, op2);
+            }
         }
-        else if (op1->IsCnsIntOrI())
+
+        if (!op2->isContained() && IsContainableMemoryOp(op1))
         {
-            op2->SetRegOptional();
+            isSafeToContainOp1 = IsSafeToContainMem(cmp, op1);
+            if (isSafeToContainOp1)
+            {
+                MakeSrcContained(cmp, op1);
+            }
         }
-        else
+
+        if (!op1->isContained() && !op2->isContained())
         {
             // One of op1 or op2 could be marked as reg optional
             // to indicate that codegen can still generate code
             // if one of them is on stack.
-            PreferredRegOptionalOperand(cmp)->SetRegOptional();
+            GenTree* regOptionalCandidate = op1->IsCnsIntOrI() ? op2 : PreferredRegOptionalOperand(cmp);
+
+            // IsSafeToContainMem is expensive so we call it at most once for each operand
+            // in this method. If we already called IsSafeToContainMem, it must have returned false;
+            // otherwise, the corresponding operand (op1 or op2) would be contained.
+            bool setRegOptional = (regOptionalCandidate == op1) ? isSafeToContainOp1 && IsSafeToContainMem(cmp, op1)
+                                                                : isSafeToContainOp2 && IsSafeToContainMem(cmp, op2);
+            if (setRegOptional)
+            {
+                regOptionalCandidate->SetRegOptional();
+            }
         }
     }
 }
@@ -2056,11 +2146,6 @@ void Lowering::ContainCheckBinary(GenTreeOp* node)
         return;
     }
 
-    // We're not marking a constant hanging on the left of an add
-    // as containable so we assign it to a register having CQ impact.
-    // TODO-XArch-CQ: Detect this case and support both generating a single instruction
-    // for GT_ADD(Constant, SomeTree)
-
     GenTree* op1 = node->gtOp1;
     GenTree* op2 = node->gtOp2;
 
@@ -2068,9 +2153,11 @@ void Lowering::ContainCheckBinary(GenTreeOp* node)
     // In case of memory-op, we can encode it directly provided its type matches with 'tree' type.
     // This is because during codegen, type of 'tree' is used to determine emit Type size. If the types
     // do not match, they get normalized (i.e. sign/zero extended) on load into a register.
-    bool     directlyEncodable = false;
-    bool     binOpInRMW        = false;
-    GenTree* operand           = nullptr;
+    bool     directlyEncodable  = false;
+    bool     binOpInRMW         = false;
+    GenTree* operand            = nullptr;
+    bool     isSafeToContainOp1 = true;
+    bool     isSafeToContainOp2 = true;
 
     if (IsContainableImmed(node, op2))
     {
@@ -2083,22 +2170,34 @@ void Lowering::ContainCheckBinary(GenTreeOp* node)
         if (!binOpInRMW)
         {
             const unsigned operatorSize = genTypeSize(node->TypeGet());
-            if (IsContainableMemoryOp(op2) && (genTypeSize(op2->TypeGet()) == operatorSize))
+            if ((genTypeSize(op2->TypeGet()) == operatorSize) && IsContainableMemoryOp(op2))
             {
-                directlyEncodable = true;
-                operand           = op2;
+                isSafeToContainOp2 = IsSafeToContainMem(node, op2);
+                if (isSafeToContainOp2)
+                {
+                    directlyEncodable = true;
+                    operand           = op2;
+                }
             }
-            else if (node->OperIsCommutative())
+
+            if ((operand == nullptr) && node->OperIsCommutative())
             {
-                if (IsContainableImmed(node, op1) ||
-                    (IsContainableMemoryOp(op1) && (genTypeSize(op1->TypeGet()) == operatorSize) &&
-                     IsSafeToContainMem(node, op1)))
+                // If it is safe, we can reverse the order of operands of commutative operations for efficient
+                // codegen
+                if (IsContainableImmed(node, op1))
                 {
-                    // If it is safe, we can reverse the order of operands of commutative operations for efficient
-                    // codegen
                     directlyEncodable = true;
                     operand           = op1;
                 }
+                else if ((genTypeSize(op1->TypeGet()) == operatorSize) && IsContainableMemoryOp(op1))
+                {
+                    isSafeToContainOp1 = IsSafeToContainMem(node, op1);
+                    if (isSafeToContainOp1)
+                    {
+                        directlyEncodable = true;
+                        operand           = op1;
+                    }
+                }
             }
         }
     }
@@ -2113,7 +2212,14 @@ void Lowering::ContainCheckBinary(GenTreeOp* node)
         // If this binary op neither has contained operands, nor is a
         // Read-Modify-Write (RMW) operation, we can mark its operands
         // as reg optional.
-        SetRegOptionalForBinOp(node);
+
+        // IsSafeToContainMem is expensive so we call it at most once for each operand
+        // in this method. If we already called IsSafeToContainMem, it must have returned false;
+        // otherwise, directlyEncodable would be true.
+        isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1);
+        isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2);
+
+        SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2);
     }
 }
 
@@ -3050,12 +3156,23 @@ void Lowering::ContainCheckFloatBinary(GenTreeOp* node)
     // everything is made explicit by adding casts.
     assert(op1->TypeGet() == op2->TypeGet());
 
-    if (IsContainableMemoryOp(op2) || op2->IsCnsNonZeroFltOrDbl())
+    bool isSafeToContainOp1 = true;
+    bool isSafeToContainOp2 = true;
+
+    if (op2->IsCnsNonZeroFltOrDbl())
     {
         MakeSrcContained(node, op2);
     }
-    else if (node->OperIsCommutative() &&
-             (op1->IsCnsNonZeroFltOrDbl() || (IsContainableMemoryOp(op1) && IsSafeToContainMem(node, op1))))
+    else if (IsContainableMemoryOp(op2))
+    {
+        isSafeToContainOp2 = IsSafeToContainMem(node, op2);
+        if (isSafeToContainOp2)
+        {
+            MakeSrcContained(node, op2);
+        }
+    }
+
+    if (!op2->isContained() && node->OperIsCommutative())
     {
         // Though we have GT_ADD(op1=memOp, op2=non-memOp, we try to reorder the operands
         // as long as it is safe so that the following efficient code sequence is generated:
@@ -3065,12 +3182,30 @@ void Lowering::ContainCheckFloatBinary(GenTreeOp* node)
         // Instead of
         //      movss op1Reg, [memOp]; addss/sd targetReg, Op2Reg  (if op1Reg == targetReg) OR
         //      movss op1Reg, [memOp]; movaps targetReg, op1Reg, addss/sd targetReg, Op2Reg
-        MakeSrcContained(node, op1);
+
+        if (op1->IsCnsNonZeroFltOrDbl())
+        {
+            MakeSrcContained(node, op1);
+        }
+        else if (IsContainableMemoryOp(op1))
+        {
+            isSafeToContainOp1 = IsSafeToContainMem(node, op1);
+            if (isSafeToContainOp1)
+            {
+                MakeSrcContained(node, op1);
+            }
+        }
     }
-    else
+
+    if (!op1->isContained() && !op2->isContained())
     {
         // If there are no containable operands, we can make an operand reg optional.
-        SetRegOptionalForBinOp(node);
+        // IsSafeToContainMem is expensive so we call it at most once for each operand
+        // in this method. If we already called IsSafeToContainMem, it must have returned false;
+        // otherwise, the corresponding operand (op1 or op2) would be contained.
+        isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1);
+        isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2);
+        SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2);
     }
 }
 

src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.cs

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+//
+// GitHub12398: Lowering is inconsistent in checking safety of RegOptional.
+
+using System;
+using System.Runtime.CompilerServices;
+
+struct S0
+{
+    public sbyte F1;
+    public sbyte F2;
+}
+
+class GitHub_12398
+{
+    static int Main()
+    {
+        int result = 100;
+        if (TestBinary() != 0) {
+            Console.WriteLine("Failed TestBinary");
+            result = -1;
+        }
+
+        if (TestCompare()) {
+            Console.WriteLine("Failed TestCompare");
+            result = -1;
+        }
+
+        if (TestMul() != 1) {
+            Console.WriteLine("Failed TestMul");
+            result = -1;
+        }
+
+        if (TestMulTypeSize() != 0) {
+            Console.WriteLine ("Failed TestMulTypeSize");
+            result = -1;
+        }
+
+        if (result == 100) {
+            Console.WriteLine("PASSED");
+        }
+        else {
+            Console.WriteLine("FAILED");
+        }
+
+        return result;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static long TestBinary()
+    {
+        long l = 0;
+        long result = l ^ System.Threading.Interlocked.Exchange(ref l, 1);
+        return result;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static bool TestCompare()
+    {
+        long l = 0;
+        bool result = l > System.Threading.Interlocked.Exchange(ref l, 1);
+        return result;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static long TestMul()
+    {
+        long l = 1;
+        long result = l * System.Threading.Interlocked.Exchange(ref l, 0);
+        return result;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static int TestMulTypeSize()
+    {
+        S0 s = new S0();
+        s.F2--;
+        int i = System.Threading.Volatile.Read(ref s.F1) * 100;
+        return i;
+    }
+}
+

src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.csproj

+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{ADEEA3D1-B67B-456E-8F2B-6DCCACC2D34C}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType></DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>