[RISC-V] Move comparison transformations from codegen to lowering (#118270)

Codegen was not a good place for these transformations, it resulted in
contorted logic and complicated register allocation. Every type of
comparison benefits: branch, floating, and integer but that was
especially true for non-branch integer comparisons -- only less-than is
available so any other comparison must be achieved with additional
operations which are much easier to insert/transform to at node level.

Inserting sign-extensions (and recognizing when it is necessary) for
comparands is also easier at node level. This PR introduces only simple
local checks, more comprehensive sign-extension elimination will be
attempted in subsequent PRs.

Part of #84834, cc @dotnet/samsung

Author: tomeksowi

src/coreclr/jit/codegenriscv64.cpp

@@ -738,7 +738,7 @@ void emitter::emitIns_R_R_I(
         (INS_lb <= ins && INS_lhu >= ins) || INS_ld == ins || INS_lw == ins || INS_jalr == ins || INS_fld == ins ||
         INS_flw == ins || INS_slli_uw == ins || INS_rori == ins || INS_roriw == ins)
     {
-        assert(isGeneralRegister(reg2));
+        assert(isGeneralRegisterOrR0(reg2));
         code |= (reg1 & 0x1f) << 7; // rd
         code |= reg2 << 15;         // rs1
         code |= imm << 20;          // imm
@@ -5433,15 +5433,6 @@ regNumber emitter::emitInsTernary(instruction ins, emitAttr attr, GenTree* dst,
                     // n * n bytes will store n bytes result
                     emitIns_R_R_R(ins, attr, dstReg, src1Reg, src2Reg);
 
-                    if ((dst->gtFlags & GTF_UNSIGNED) != 0)
-                    {
-                        if (attr == EA_4BYTE)
-                        {
-                            emitIns_R_R_I(INS_slli, EA_8BYTE, dstReg, dstReg, 32);
-                            emitIns_R_R_I(INS_srli, EA_8BYTE, dstReg, dstReg, 32);
-                        }
-                    }
-
                     if (needCheckOv)
                     {
                         assert(tempReg != dstReg);

src/coreclr/jit/emitriscv64.cpp

@@ -4507,7 +4507,18 @@ GenTree* Lowering::LowerCompare(GenTree* cmp)
             cmp->gtFlags |= GTF_UNSIGNED;
         }
     }
-#endif // TARGET_XARCH
+#elif defined(TARGET_RISCV64)
+    if (varTypeUsesIntReg(cmp->gtGetOp1()))
+    {
+        if (GenTree* next = LowerSavedIntegerCompare(cmp); next != cmp)
+            return next;
+
+        // Integer comparisons are full-register only.
+        SignExtendIfNecessary(&cmp->AsOp()->gtOp1);
+        SignExtendIfNecessary(&cmp->AsOp()->gtOp2);
+    }
+#endif // TARGET_RISCV64
+
     ContainCheckCompare(cmp->AsOp());
     return cmp->gtNext;
 }
@@ -7785,7 +7796,7 @@ bool Lowering::LowerUnsignedDivOrMod(GenTreeOp* divMod)
         {
             divMod->ChangeOper(GT_GE);
             divMod->gtFlags |= GTF_UNSIGNED;
-            ContainCheckNode(divMod);
+            LowerNode(divMod);
             return true;
         }
     }

src/coreclr/jit/lower.cpp

@@ -156,9 +156,13 @@ class Lowering final : public Phase
 #ifndef TARGET_64BIT
     GenTree* DecomposeLongCompare(GenTree* cmp);
 #endif
-    GenTree*   OptimizeConstCompare(GenTree* cmp);
-    GenTree*   LowerCompare(GenTree* cmp);
-    GenTree*   LowerJTrue(GenTreeOp* jtrue);
+    GenTree* OptimizeConstCompare(GenTree* cmp);
+    GenTree* LowerCompare(GenTree* cmp);
+    GenTree* LowerJTrue(GenTreeOp* jtrue);
+#ifdef TARGET_RISCV64
+    GenTree* LowerSavedIntegerCompare(GenTree* cmp);
+    void     SignExtendIfNecessary(GenTree** arg);
+#endif
     GenTree*   LowerSelect(GenTreeConditional* cond);
     bool       TryLowerConditionToFlagsNode(GenTree*      parent,
                                             GenTree*      condition,

src/coreclr/jit/lower.h

@@ -64,24 +64,19 @@ bool Lowering::IsContainableImmed(GenTree* parentNode, GenTree* childNode) const
 
         switch (parentNode->OperGet())
         {
-            case GT_EQ:
-            case GT_NE:
-                return emitter::isValidSimm12(-immVal) || (immVal == -2048);
-
-            case GT_LE: // a <= N  ->  a < N+1
-            case GT_GT: // a > N  ->  !(a <= N)  ->  !(a < N+1)
-                immVal += 1;
-                FALLTHROUGH;
             case GT_LT:
             case GT_GE:
             case GT_ADD:
             case GT_AND:
             case GT_OR:
             case GT_XOR:
                 return emitter::isValidSimm12(immVal);
-            case GT_JCMP:
-                return true;
 
+            case GT_EQ:
+            case GT_NE:
+            case GT_GT:
+            case GT_LE:
+            case GT_JCMP:
             case GT_CMPXCHG:
             case GT_XORR:
             case GT_XAND:
@@ -133,57 +128,158 @@ GenTree* Lowering::LowerMul(GenTreeOp* mul)
 //
 GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue)
 {
-    GenTree*     op = jtrue->gtGetOp1();
-    GenCondition cond;
-    GenTree*     cmpOp1;
-    GenTree*     cmpOp2;
-
-    assert(!op->OperIsCompare() || op->OperIsCmpCompare()); // We do not expect any other relops on RISCV64
+    GenTree* cmp = jtrue->gtGetOp1();
+    assert(!cmp->OperIsCompare() || cmp->OperIsCmpCompare()); // We do not expect any other relops on RISCV64
 
-    if (op->OperIsCompare() && !varTypeIsFloating(op->gtGetOp1()))
+    // for RISCV64's compare and condition-branch instructions, it's very similar to the IL instructions.
+    jtrue->ChangeOper(GT_JCMP);
+    GenTreeOpCC* jcmp = jtrue->AsOpCC();
+    if (cmp->OperIsCompare() && !varTypeIsFloating(cmp->gtGetOp1()))
     {
-        cond = GenCondition::FromRelop(op);
-
-        cmpOp1 = op->gtGetOp1();
-        cmpOp2 = op->gtGetOp2();
-
-        // We will fall through and turn this into a JCMP(op1, op2, kind), but need to remove the relop here.
-        BlockRange().Remove(op);
+        jcmp->gtCondition = GenCondition::FromIntegralRelop(cmp);
+        jcmp->gtOp1       = cmp->gtGetOp1();
+        jcmp->gtOp2       = cmp->gtGetOp2();
+        BlockRange().Remove(cmp);
     }
     else
     {
+        // branch if (cond)  --->  branch if (cond != 0)
         GenCondition::Code code = GenCondition::NE;
-        if (op->OperIsCompare() && varTypeIsFloating(op->gtGetOp1()) && (op->gtFlags & GTF_RELOP_NAN_UN) != 0)
+        if (cmp->OperIsCompare() && varTypeIsFloating(cmp->gtGetOp1()) && (cmp->gtFlags & GTF_RELOP_NAN_UN) != 0)
         {
             // Unordered floating-point comparisons are achieved by neg'ing the ordered counterparts. Avoid that by
             // reversing both the FP comparison and the zero-comparison fused with the branch.
-            op->ChangeOper(GenTree::ReverseRelop(op->OperGet()));
-            op->gtFlags &= ~GTF_RELOP_NAN_UN;
+            cmp->ChangeOper(GenTree::ReverseRelop(cmp->OperGet()));
+            cmp->gtFlags &= ~GTF_RELOP_NAN_UN;
             code = GenCondition::EQ;
         }
-        cond = GenCondition(code);
+        jcmp->gtCondition = GenCondition(code);
+        jcmp->gtOp1       = cmp;
+        jcmp->gtOp2       = comp->gtNewZeroConNode(cmp->TypeGet());
+        BlockRange().InsertBefore(jcmp, jcmp->gtOp2);
+    }
 
-        cmpOp1 = op;
-        cmpOp2 = comp->gtNewZeroConNode(cmpOp1->TypeGet());
+    // Comparisons fused with branches don't have immediates, re-evaluate containment for 'zero' register
+    if (!CheckImmedAndMakeContained(jcmp, jcmp->gtOp2))
+        jcmp->gtOp2->ClearContained();
 
-        BlockRange().InsertBefore(jtrue, cmpOp2);
+    return jcmp->gtNext;
+}
 
-        // Fall through and turn this into a JCMP(op1, 0, NE).
-    }
+//------------------------------------------------------------------------
+// LowerSavedIntegerCompare: lowers a integer comparison saved to a register so that it matches the available
+// instructions better
+//
+// Arguments:
+//    cmp - the integer comparison to lower
+//
+// Return Value:
+//    The original compare node if lowering should proceed as usual or the next node to lower if the compare node was
+//    changed in such a way that lowering is no longer needed.
+//
+GenTree* Lowering::LowerSavedIntegerCompare(GenTree* cmp)
+{
+    // Branches have a full range of comparisons, these transformations would be counter-productive
+    LIR::Use cmpUse;
+    if (!BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE))
+        return cmp;
 
-    // for RISCV64's compare and condition-branch instructions,
-    // it's very similar to the IL instructions.
-    jtrue->ChangeOper(GT_JCMP);
-    jtrue->gtOp1                 = cmpOp1;
-    jtrue->gtOp2                 = cmpOp2;
-    jtrue->AsOpCC()->gtCondition = cond;
+    GenTree*& left  = cmp->AsOp()->gtOp1;
+    GenTree*& right = cmp->AsOp()->gtOp2;
+    assert(cmp->OperIsCmpCompare() && varTypeUsesIntReg(left));
+
+    if (cmp->OperIs(GT_EQ, GT_NE) && !right->IsIntegralConst(0))
+    {
+        // Only equality with zero is supported
+        // a == b  --->  (a - b) == 0
+        var_types  type = genActualTypeIsInt(left) ? TYP_INT : TYP_I_IMPL;
+        genTreeOps oper = GT_SUB;
+        if (right->IsIntegralConst() && !right->AsIntCon()->ImmedValNeedsReloc(comp))
+        {
+            INT64 value  = right->AsIntConCommon()->IntegralValue();
+            INT64 minVal = (type == TYP_INT) ? INT_MIN : SSIZE_T_MIN;
+
+            const INT64 min12BitImm = -2048;
+            if (value == min12BitImm)
+            {
+                // (a - C) == 0 ---> (a ^ C) == 0
+                oper = GT_XOR;
+            }
+            else if (!right->TypeIs(TYP_BYREF) && value != minVal)
+            {
+                // a - C  --->  a + (-C)
+                oper = GT_ADD;
+                right->AsIntConCommon()->SetIntegralValue(-value);
+            }
+        }
+        left  = comp->gtNewOperNode(oper, type, left, right);
+        right = comp->gtNewZeroConNode(type);
+        BlockRange().InsertBefore(cmp, left, right);
+        ContainCheckBinary(left->AsOp());
+    }
 
-    if (cmpOp2->IsCnsIntOrI())
+    if (!right->TypeIs(TYP_BYREF) && right->IsIntegralConst() && !right->AsIntConCommon()->ImmedValNeedsReloc(comp))
     {
-        cmpOp2->SetContained();
+        if (cmp->OperIs(GT_LE, GT_GE))
+        {
+            // a <= C  --->  a < C+1
+            // a >= C  --->  a > C-1
+            INT64 value = right->AsIntConCommon()->IntegralValue();
+
+            bool isOverflow;
+            if (cmp->OperIs(GT_LE))
+            {
+                isOverflow = genActualTypeIsInt(left)
+                                 ? CheckedOps::AddOverflows((INT32)value, (INT32)1, cmp->IsUnsigned())
+                                 : CheckedOps::AddOverflows((INT64)value, (INT64)1, cmp->IsUnsigned());
+            }
+            else
+            {
+                isOverflow = genActualTypeIsInt(left)
+                                 ? CheckedOps::SubOverflows((INT32)value, (INT32)1, cmp->IsUnsigned())
+                                 : CheckedOps::SubOverflows((INT64)value, (INT64)1, cmp->IsUnsigned());
+            }
+            if (!isOverflow)
+            {
+                right->AsIntConCommon()->SetIntegralValue(cmp->OperIs(GT_LE) ? value + 1 : value - 1);
+                cmp->SetOperRaw(cmp->OperIs(GT_LE) ? GT_LT : GT_GT);
+            }
+        }
+
+        if (cmp->OperIs(GT_LT) && right->IsIntegralConst(0) && !cmp->IsUnsigned())
+        {
+            // a < 0 (signed)  --->  shift the sign bit into the lowest bit
+            cmp->SetOperRaw(GT_RSZ);
+            cmp->ChangeType(genActualType(left));
+            right->AsIntConCommon()->SetIntegralValue(genTypeSize(cmp) * BITS_PER_BYTE - 1);
+            right->SetContained();
+            return cmp->gtNext;
+        }
     }
+    return cmp;
+}
+
+//------------------------------------------------------------------------
+// SignExtendIfNecessary: inserts a 32-bit sign extension unless the argument is full-register or is known to be
+// implemented with a sign-extending instruction.
+//
+// Arguments:
+//    arg - the argument to sign-extend
+//
+void Lowering::SignExtendIfNecessary(GenTree** arg)
+{
+    assert(varTypeUsesIntReg(*arg));
+    if (!genActualTypeIsInt(*arg))
+        return;
+
+    if ((*arg)->OperIs(GT_ADD, GT_SUB, GT_MUL, GT_MOD, GT_UMOD, GT_DIV, GT_UDIV, GT_CNS_INT))
+        return;
 
-    return jtrue->gtNext;
+    if ((*arg)->OperIsShiftOrRotate() || (*arg)->OperIsCmpCompare() || (*arg)->OperIsAtomicOp())
+        return;
+
+    *arg = comp->gtNewCastNode(TYP_I_IMPL, *arg, false, TYP_I_IMPL);
+    BlockRange().InsertAfter((*arg)->gtGetOp1(), *arg);
 }
 
 //------------------------------------------------------------------------
@@ -1140,6 +1236,9 @@ void Lowering::ContainCheckCast(GenTreeCast* node)
 //
 void Lowering::ContainCheckCompare(GenTreeOp* cmp)
 {
+    if (cmp->gtOp1->IsIntegralConst(0) && !cmp->gtOp1->AsIntCon()->ImmedValNeedsReloc(comp))
+        MakeSrcContained(cmp, cmp->gtOp1); // use 'zero' register
+
     CheckImmedAndMakeContained(cmp, cmp->gtOp2);
 }
 

src/coreclr/jit/lowerriscv64.cpp

@@ -9065,23 +9065,27 @@ void LinearScan::handleOutgoingCriticalEdges(BasicBlock* block)
 
         if (lastNode->OperIs(GT_JTRUE, GT_JCMP, GT_JTEST))
         {
-            GenTree* op = lastNode->gtGetOp1();
-            consumedRegs |= genSingleTypeRegMask(op->GetRegNum());
-
-            if (op->OperIs(GT_COPY))
-            {
-                GenTree* srcOp = op->gtGetOp1();
-                consumedRegs |= genSingleTypeRegMask(srcOp->GetRegNum());
-            }
-            else if (op->IsLocal())
+            assert(!lastNode->OperIs(GT_JTRUE) || !lastNode->gtGetOp1()->isContained());
+            if (!lastNode->gtGetOp1()->isContained())
             {
-                GenTreeLclVarCommon* lcl = op->AsLclVarCommon();
-                terminatorNodeLclVarDsc  = &compiler->lvaTable[lcl->GetLclNum()];
+                GenTree* op = lastNode->gtGetOp1();
+                consumedRegs |= genSingleTypeRegMask(op->GetRegNum());
+
+                if (op->OperIs(GT_COPY))
+                {
+                    GenTree* srcOp = op->gtGetOp1();
+                    consumedRegs |= genSingleTypeRegMask(srcOp->GetRegNum());
+                }
+                else if (op->IsLocal())
+                {
+                    GenTreeLclVarCommon* lcl = op->AsLclVarCommon();
+                    terminatorNodeLclVarDsc  = &compiler->lvaTable[lcl->GetLclNum()];
+                }
             }
 
             if (lastNode->OperIs(GT_JCMP, GT_JTEST) && !lastNode->gtGetOp2()->isContained())
             {
-                op = lastNode->gtGetOp2();
+                GenTree* op = lastNode->gtGetOp2();
                 consumedRegs |= genSingleTypeRegMask(op->GetRegNum());
 
                 if (op->OperIs(GT_COPY))

src/coreclr/jit/lsra.cpp

@@ -202,11 +202,6 @@ int LinearScan::BuildNode(GenTree* tree)
             srcCount = BuildOperandUses(tree->gtGetOp1());
             break;
 
-        case GT_JTRUE:
-            srcCount = 0;
-            assert(dstCount == 0);
-            break;
-
         case GT_JMP:
             srcCount = 0;
             assert(dstCount == 0);
@@ -440,30 +435,6 @@ int LinearScan::BuildNode(GenTree* tree)
         case GT_LE:
         case GT_GE:
         case GT_GT:
-        {
-            var_types op1Type = genActualType(tree->gtGetOp1()->TypeGet());
-            if (!varTypeIsFloating(op1Type))
-            {
-                emitAttr cmpSize = EA_ATTR(genTypeSize(op1Type));
-                if (cmpSize == EA_4BYTE)
-                {
-                    GenTree* op2 = tree->gtGetOp2();
-
-                    bool isUnsigned    = (tree->gtFlags & GTF_UNSIGNED) != 0;
-                    bool useAddSub     = !(!tree->OperIs(GT_EQ, GT_NE) || op2->IsIntegralConst(-2048));
-                    bool useShiftRight = !isUnsigned && ((tree->OperIs(GT_LT) && op2->IsIntegralConst(0)) ||
-                                                         (tree->OperIs(GT_LE) && op2->IsIntegralConst(-1)));
-                    bool useLoadImm    = isUnsigned && ((tree->OperIs(GT_LT, GT_GE) && op2->IsIntegralConst(0)) ||
-                                                     (tree->OperIs(GT_LE, GT_GT) && op2->IsIntegralConst(-1)));
-
-                    if (!useAddSub && !useShiftRight && !useLoadImm)
-                        buildInternalIntRegisterDefForNode(tree);
-                }
-            }
-            buildInternalRegisterUses();
-        }
-            FALLTHROUGH;
-
         case GT_JCMP:
             srcCount = BuildCmp(tree);
             break;