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

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 #12398.

diff --git a/src/jit/lower.cpp b/src/jit/lower.cpp
index fb5fab2..c6648b8 100644 (file)
--- a/src/jit/lower.cpp
+++ b/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,

diff --git a/src/jit/lower.h b/src/jit/lower.h
index c78e26b..c57e732 100644 (file)
--- a/src/jit/lower.h
+++ b/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)

diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp
index c748da0..de22b59 100644 (file)
--- a/src/jit/lowerxarch.cpp
+++ b/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);
     }
 }
 

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.cs b/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.cs
new file mode 100644 (file)
index 0000000..69057f3
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.cs
@@ -0,0 +1,84 @@
+// 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;
+    }
+}
+

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.csproj
new file mode 100644 (file)
index 0000000..67d0b29
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.csproj
@@ -0,0 +1,34 @@
+<?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>