From 0c89442a570156e6e52813996bef222fc6c15445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Mon, 14 Jul 2025 18:40:31 +0200 Subject: [PATCH 01/36] Move FP comparisons swap & reverse to lowering --- src/coreclr/jit/codegenriscv64.cpp | 19 +++-------- src/coreclr/jit/lower.cpp | 32 ++++++++++++++++++- .../JIT/CodeGenBringUpTests/JTrueGeFP.cs | 7 ++++ 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 6deaa25f599831..3efbec688273d4 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -1691,7 +1691,8 @@ void CodeGen::genCodeForNegNot(GenTree* tree) else if (tree->OperIs(GT_NOT)) { assert(!varTypeIsFloating(targetType)); - GetEmitter()->emitIns_R_R_I(INS_xori, attr, targetReg, operandReg, -1); + ssize_t mask = tree->gtGetOp1()->OperIsCmpCompare() ? 1 : -1; + GetEmitter()->emitIns_R_R_I(INS_xori, attr, targetReg, operandReg, mask); } genProduceReg(tree); @@ -3131,20 +3132,10 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) { assert(!op2->isContainedIntOrIImmed()); assert(op1Type == op2Type); - genTreeOps oper = tree->OperGet(); + noway_assert((tree->gtFlags & GTF_RELOP_NAN_UN) == 0); - bool isUnordered = (tree->gtFlags & GTF_RELOP_NAN_UN) != 0; - if (isUnordered) - { - oper = GenTree::ReverseRelop(oper); - } - if (oper == GT_GT || oper == GT_GE) - { - oper = GenTree::SwapRelop(oper); - std::swap(op1, op2); - } instruction instr = INS_none; - switch (oper) + switch (tree->OperGet()) { case GT_LT: instr = (cmpSize == EA_4BYTE) ? INS_flt_s : INS_flt_d; @@ -3159,8 +3150,6 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) unreached(); } emit->emitIns_R_R_R(instr, cmpSize, targetReg, op1->GetRegNum(), op2->GetRegNum()); - if (isUnordered) - emit->emitIns_R_R_I(INS_xori, EA_8BYTE, targetReg, targetReg, 1); } else { diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 49aa7e43454d60..25cfa938194c39 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4447,7 +4447,37 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) cmp->gtFlags |= GTF_UNSIGNED; } } -#endif // TARGET_XARCH +#elif defined(TARGET_RISCV64) + GenTree*& left = cmp->AsOp()->gtOp1; + GenTree*& right = cmp->AsOp()->gtOp2; + + LIR::Use cmpUse; + if (BlockRange().TryGetUse(cmp, &cmpUse) && !cmpUse.User()->OperIs(GT_JTRUE)) + { + if (varTypeIsFloating(left)) + { + assert(left->TypeIs(right->TypeGet())); + if ((cmp->gtFlags & GTF_RELOP_NAN_UN) != 0) + { + comp->gtReverseCond(cmp); + GenTree* notOp = comp->gtNewOperNode(GT_NOT, cmp->gtType, cmp); + BlockRange().InsertAfter(cmp, notOp); + cmpUse.ReplaceWith(notOp); + } + } + } + + if (varTypeIsFloating(left)) + { + bool isUnordered = (cmp->gtFlags & GTF_RELOP_NAN_UN) != 0; + if ((!isUnordered && cmp->OperIs(GT_GT, GT_GE)) || (isUnordered && cmp->OperIs(GT_LE, GT_LT))) + { + cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); + std::swap(left, right); + } + } +#endif // TARGET_RISCV64 + ContainCheckCompare(cmp->AsOp()); return cmp->gtNext; } diff --git a/src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs b/src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs index 15c22ec83d8f2a..c2a63451083a1d 100644 --- a/src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs +++ b/src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs @@ -24,6 +24,12 @@ public static int JTrueGeFP(float x) return returnValue; } + + [MethodImplAttribute(MethodImplOptions.NoInlining)] + public static bool JTrueGeFP_bool(float x) + { + return (x >= 2f); + } [Fact] public static int TestEntryPoint() @@ -34,6 +40,7 @@ public static int TestEntryPoint() if (JTrueGeFP(0f) != 2) returnValue = Fail; if (JTrueGeFP(1f) != 3) returnValue = Fail; if (JTrueGeFP(2f) != 4) returnValue = Fail; + if (!JTrueGeFP_bool(2f)) returnValue = Fail; return returnValue; From 6d19c645783928a70ec8a78005f5709da9df2ce0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Wed, 16 Jul 2025 17:30:19 +0200 Subject: [PATCH 02/36] Lower integer comparisons --- src/coreclr/jit/lower.cpp | 81 +++++++++++++++++++++++++------- src/coreclr/jit/lower.h | 9 ++-- src/coreclr/jit/lowerriscv64.cpp | 39 ++++++++++++--- 3 files changed, 103 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 25cfa938194c39..fe7184e1f0c76c 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4448,32 +4448,79 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) } } #elif defined(TARGET_RISCV64) - GenTree*& left = cmp->AsOp()->gtOp1; - GenTree*& right = cmp->AsOp()->gtOp2; - + // Note: branch instructions are fused with a full range of integer comparisons. However, comparisons which save the + // result to a register are covered with fewer instructions: + // * for integer: "<" (with unsigned and immediate variants), + // * for floating: "<", "<=", "==" (ordered only, register only), + // so the rest is achieved through various transformations. LIR::Use cmpUse; if (BlockRange().TryGetUse(cmp, &cmpUse) && !cmpUse.User()->OperIs(GT_JTRUE)) { + GenTree*& left = cmp->AsOp()->gtOp1; + GenTree*& right = cmp->AsOp()->gtOp2; + + bool isReversed = false; if (varTypeIsFloating(left)) { - assert(left->TypeIs(right->TypeGet())); - if ((cmp->gtFlags & GTF_RELOP_NAN_UN) != 0) + // a CMP b (unordered) ---> !(a reversedCMP b (ordered)) + isReversed = LowerAndReverseFloatingCompare(cmp); + } + else + { + assert(varTypeUsesIntReg(left)); + if (cmp->OperIs(GT_EQ, GT_NE)) { - comp->gtReverseCond(cmp); - GenTree* notOp = comp->gtNewOperNode(GT_NOT, cmp->gtType, cmp); - BlockRange().InsertAfter(cmp, notOp); - cmpUse.ReplaceWith(notOp); + if (!right->IsIntegralConst(0)) + { + // a == b ---> (a - b) == 0 + + // We may be comparing GC types, the type of the operands is GC but the type of the difference is + // not. The objects may be relocated but it's OK as the diff is used to only check against zero. + var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_LONG; + left = comp->gtNewOperNode(GT_SUB, type, left, right); + right = comp->gtNewZeroConNode(type); + BlockRange().InsertBefore(cmp, left); + BlockRange().InsertBefore(cmp, right); + ContainCheckBinary(left->AsOp()); + } + // a == 0 ---> a <= 0 (unsigned) + cmp->SetOperRaw(cmp->OperIs(GT_EQ) ? GT_LE : GT_GT); + cmp->SetUnsigned(); } - } - } + assert(!cmp->OperIs(GT_EQ, GT_NE)); - if (varTypeIsFloating(left)) - { - bool isUnordered = (cmp->gtFlags & GTF_RELOP_NAN_UN) != 0; - if ((!isUnordered && cmp->OperIs(GT_GT, GT_GE)) || (isUnordered && cmp->OperIs(GT_LE, GT_LT))) + if (right->IsIntegralConst() && cmp->OperIs(GT_LE, GT_GT)) + { + // a <= C ---> a < C+1 + if (cmp->IsUnsigned() && right->IsIntegralConst(SIZE_T_MAX)) + { + // (max+1) wraps to 0 so unsigned (a < max+1) would be always false instead of true + BlockRange().Remove(left); + BlockRange().Remove(right); + cmp->BashToConst(cmp->OperIs(GT_LE) ? 1 : 0); + return cmp->gtNext; + } + INT64 value = right->AsIntConCommon()->IntegralValue(); + right->AsIntConCommon()->SetIntegralValue(value + 1); + cmp->SetOperRaw(cmp->OperIs(GT_LE) ? GT_LT : GT_GE); + } + // a <= b ---> !(a > b) + isReversed = cmp->OperIs(GT_LE, GT_GE); + if (isReversed) + cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); + + if (cmp->OperIs(GT_GT)) + { + cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); + std::swap(cmp->AsOp()->gtOp1, cmp->AsOp()->gtOp2); + } + assert(cmp->OperIs(GT_LT)); + } + if (isReversed) { - cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); - std::swap(left, right); + GenTree* notOp = comp->gtNewOperNode(GT_NOT, cmp->gtType, cmp); + BlockRange().InsertAfter(cmp, notOp); + cmpUse.ReplaceWith(notOp); } } #endif // TARGET_RISCV64 diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index b3afd8cceaeb28..dd4d2d55578f56 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -156,9 +156,12 @@ 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 + bool LowerAndReverseFloatingCompare(GenTree* cmp); +#endif GenTree* LowerSelect(GenTreeConditional* cond); bool TryLowerConditionToFlagsNode(GenTree* parent, GenTree* condition, diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 7f397d9272792f..83f1400a384f90 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -153,13 +153,10 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) else { GenCondition::Code code = GenCondition::NE; - if (op->OperIsCompare() && varTypeIsFloating(op->gtGetOp1()) && (op->gtFlags & GTF_RELOP_NAN_UN) != 0) + if (op->OperIsCompare() && varTypeIsFloating(op->gtGetOp1())) { - // 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; - code = GenCondition::EQ; + if (LowerAndReverseFloatingCompare(op)) + code = GenCondition::EQ; } cond = GenCondition(code); @@ -186,6 +183,36 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) return jtrue->gtNext; } +//------------------------------------------------------------------------ +// LowerAndReverseFloatingCompare: lowers and reverses a floating-point comparison so that its oper matches the +// available instructions +// +// Arguments: +// cmp - the floating-point comparison to lower +// +// Returns: +// Whether the comparison was reversed. +// +bool Lowering::LowerAndReverseFloatingCompare(GenTree* cmp) +{ + assert(cmp->OperIsCmpCompare() && varTypeIsFloating(cmp->gtGetOp1())); + bool isUnordered = (cmp->gtFlags & GTF_RELOP_NAN_UN) != 0; + if (isUnordered) + { + // Unordered floating-point comparisons are achieved by neg'ing the ordered counterparts. Avoid that by + // reversing the FP comparison and communicating to the caller to reverse its use appropriately. + cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); + cmp->gtFlags &= ~GTF_RELOP_NAN_UN; + } + if (cmp->OperIs(GT_GT, GT_GE)) + { + cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); + std::swap(cmp->AsOp()->gtOp1, cmp->AsOp()->gtOp2); + } + assert(cmp->OperIs(GT_EQ, GT_LT, GT_LE)); + return isUnordered; +} + //------------------------------------------------------------------------ // LowerBinaryArithmetic: lowers the given binary arithmetic node. // From 369144ed297b9de100edf74904c45f4ef4cc9368 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 17 Jul 2025 15:50:58 +0200 Subject: [PATCH 03/36] Sign-extend in lowering, remove logic from codegen --- src/coreclr/jit/codegenriscv64.cpp | 136 ++--------------------------- src/coreclr/jit/lower.cpp | 22 +++-- src/coreclr/jit/lsrariscv64.cpp | 27 ------ 3 files changed, 24 insertions(+), 161 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 3efbec688273d4..6246214dbda338 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3100,7 +3100,7 @@ void CodeGen::genCkfinite(GenTree* treeNode) } //------------------------------------------------------------------------ -// genCodeForCompare: Produce code for a GT_EQ/GT_NE/GT_LT/GT_LE/GT_GE/GT_GT node. +// genCodeForCompare: Produce code for a GT_EQ/GT_LT/GT_LE node. // // Arguments: // tree - the node @@ -3110,7 +3110,6 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) GenTree* op1 = tree->gtOp1; GenTree* op2 = tree->gtOp2; var_types op1Type = genActualType(op1->TypeGet()); - var_types op2Type = genActualType(op2->TypeGet()); assert(!op1->isUsedFromMemory()); assert(!op2->isUsedFromMemory()); @@ -3118,20 +3117,17 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) emitAttr cmpSize = EA_ATTR(genTypeSize(op1Type)); assert(cmpSize == EA_4BYTE || cmpSize == EA_8BYTE); - assert(genTypeSize(op1Type) == genTypeSize(op2Type)); - emitter* emit = GetEmitter(); regNumber targetReg = tree->GetRegNum(); assert(targetReg != REG_NA); assert(!tree->TypeIs(TYP_VOID)); assert(!op1->isContainedIntOrIImmed()); - assert(tree->OperIs(GT_LT, GT_LE, GT_EQ, GT_NE, GT_GT, GT_GE)); if (varTypeIsFloating(op1Type)) { assert(!op2->isContainedIntOrIImmed()); - assert(op1Type == op2Type); + assert(op1->TypeIs(op2->TypeGet())); noway_assert((tree->gtFlags & GTF_RELOP_NAN_UN) == 0); instruction instr = INS_none; @@ -3153,134 +3149,16 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) } else { - bool isUnsigned = (tree->gtFlags & GTF_UNSIGNED) != 0; - regNumber regOp1 = op1->GetRegNum(); - + assert(tree->OperIs(GT_LT)); if (op2->isContainedIntOrIImmed()) { - ssize_t imm = op2->AsIntCon()->gtIconVal; - - bool useAddSub = !(!tree->OperIs(GT_EQ, GT_NE) || (imm == -2048)); - bool useShiftRight = - !isUnsigned && ((tree->OperIs(GT_LT) && (imm == 0)) || (tree->OperIs(GT_LE) && (imm == -1))); - bool useLoadImm = isUnsigned && ((tree->OperIs(GT_LT, GT_GE) && (imm == 0)) || - (tree->OperIs(GT_LE, GT_GT) && (imm == -1))); - - if (cmpSize == EA_4BYTE) - { - if (!useAddSub && !useShiftRight && !useLoadImm) - { - regNumber tmpRegOp1 = internalRegisters.GetSingle(tree); - assert(regOp1 != tmpRegOp1); - imm = static_cast(imm); - emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp1, regOp1); - regOp1 = tmpRegOp1; - } - } - - if (tree->OperIs(GT_EQ, GT_NE)) - { - if ((imm != 0) || (cmpSize == EA_4BYTE)) - { - instruction diff = INS_xori; - if (imm != -2048) - { - assert(useAddSub); - diff = (cmpSize == EA_4BYTE) ? INS_addiw : INS_addi; - imm = -imm; - } - emit->emitIns_R_R_I(diff, cmpSize, targetReg, regOp1, imm); - regOp1 = targetReg; - } - assert(emitter::isValidSimm12(imm)); - - if (tree->OperIs(GT_EQ)) - { - emit->emitIns_R_R_I(INS_sltiu, EA_PTRSIZE, targetReg, regOp1, 1); - } - else - { - assert(tree->OperIs(GT_NE)); - emit->emitIns_R_R_R(INS_sltu, EA_PTRSIZE, targetReg, REG_ZERO, regOp1); - } - } - else - { - assert(tree->OperIs(GT_LT, GT_LE, GT_GT, GT_GE)); - if (useLoadImm) - { - // unsigned (a <= ~0), (a >= 0) / (a > ~0), (a < 0) is always true / false - imm = tree->OperIs(GT_GE, GT_LE) ? 1 : 0; - emit->emitIns_R_R_I(INS_addi, EA_PTRSIZE, targetReg, REG_ZERO, imm); - } - else if (useShiftRight) - { - // signed (a < 0) or (a <= -1) is just the sign bit - instruction srli = (cmpSize == EA_4BYTE) ? INS_srliw : INS_srli; - emit->emitIns_R_R_I(srli, cmpSize, targetReg, regOp1, cmpSize * 8 - 1); - } - else if ((tree->OperIs(GT_GT) && (imm == 0)) || (tree->OperIs(GT_GE) && (imm == 1))) - { - instruction slt = isUnsigned ? INS_sltu : INS_slt; - emit->emitIns_R_R_R(slt, EA_PTRSIZE, targetReg, REG_ZERO, regOp1); - } - else - { - instruction slti = isUnsigned ? INS_sltiu : INS_slti; - if (tree->OperIs(GT_LE, GT_GT)) - imm += 1; - assert(emitter::isValidSimm12(imm)); - assert(!isUnsigned || (imm != 0)); // should be handled in useLoadImm - - emit->emitIns_R_R_I(slti, EA_PTRSIZE, targetReg, regOp1, imm); - - if (tree->OperIs(GT_GT, GT_GE)) - emit->emitIns_R_R_I(INS_xori, EA_PTRSIZE, targetReg, targetReg, 1); - } - } + instruction slti = tree->IsUnsigned() ? INS_sltiu : INS_slti; + emit->emitIns_R_R_I(slti, EA_PTRSIZE, targetReg, op1->GetRegNum(), op2->AsIntCon()->gtIconVal); } else { - regNumber regOp2 = op2->GetRegNum(); - - if (tree->OperIs(GT_EQ, GT_NE)) - { - instruction sub = (cmpSize == EA_4BYTE) ? INS_subw : INS_sub; - emit->emitIns_R_R_R(sub, EA_PTRSIZE, targetReg, regOp1, regOp2); - if (tree->OperIs(GT_EQ)) - { - emit->emitIns_R_R_I(INS_sltiu, EA_PTRSIZE, targetReg, targetReg, 1); - } - else - { - assert(tree->OperIs(GT_NE)); - emit->emitIns_R_R_R(INS_sltu, EA_PTRSIZE, targetReg, REG_ZERO, targetReg); - } - } - else - { - assert(tree->OperIs(GT_LT, GT_LE, GT_GT, GT_GE)); - if (cmpSize == EA_4BYTE) - { - regNumber tmpRegOp1 = REG_RA; - regNumber tmpRegOp2 = internalRegisters.GetSingle(tree); - assert(regOp1 != tmpRegOp2); - assert(regOp2 != tmpRegOp2); - emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp1, regOp1); - emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp2, regOp2); - regOp1 = tmpRegOp1; - regOp2 = tmpRegOp2; - } - - instruction slt = isUnsigned ? INS_sltu : INS_slt; - if (tree->OperIs(GT_LE, GT_GT)) - std::swap(regOp1, regOp2); - - emit->emitIns_R_R_R(slt, EA_8BYTE, targetReg, regOp1, regOp2); - - if (tree->OperIs(GT_LE, GT_GE)) - emit->emitIns_R_R_I(INS_xori, EA_PTRSIZE, targetReg, targetReg, 1); - } + instruction slt = tree->IsUnsigned() ? INS_sltu : INS_slt; + emit->emitIns_R_R_R(slt, EA_PTRSIZE, targetReg, op1->GetRegNum(), op2->GetRegNum()); } } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index fe7184e1f0c76c..d76aa389cf12d3 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4453,12 +4453,11 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) // * for integer: "<" (with unsigned and immediate variants), // * for floating: "<", "<=", "==" (ordered only, register only), // so the rest is achieved through various transformations. - LIR::Use cmpUse; + GenTree*& left = cmp->AsOp()->gtOp1; + GenTree*& right = cmp->AsOp()->gtOp2; + LIR::Use cmpUse; if (BlockRange().TryGetUse(cmp, &cmpUse) && !cmpUse.User()->OperIs(GT_JTRUE)) { - GenTree*& left = cmp->AsOp()->gtOp1; - GenTree*& right = cmp->AsOp()->gtOp2; - bool isReversed = false; if (varTypeIsFloating(left)) { @@ -4512,9 +4511,21 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) if (cmp->OperIs(GT_GT)) { cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); - std::swap(cmp->AsOp()->gtOp1, cmp->AsOp()->gtOp2); + std::swap(left, right); } assert(cmp->OperIs(GT_LT)); + + // Integer comparisons come only in full-register variants so sign-extend operands as needed + if (genActualTypeIsInt(left) && !left->OperIs(GT_ADD, GT_SUB, GT_CNS_INT)) + { + left = comp->gtNewCastNode(TYP_INT, left, false, TYP_INT); + BlockRange().InsertAfter(left->gtGetOp1(), left); + } + if (genActualTypeIsInt(right) && !right->OperIs(GT_ADD, GT_SUB, GT_CNS_INT)) + { + right = comp->gtNewCastNode(TYP_INT, right, false, TYP_INT); + BlockRange().InsertAfter(right->gtGetOp1(), right); + } } if (isReversed) { @@ -4523,6 +4534,7 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) cmpUse.ReplaceWith(notOp); } } + #endif // TARGET_RISCV64 ContainCheckCompare(cmp->AsOp()); diff --git a/src/coreclr/jit/lsrariscv64.cpp b/src/coreclr/jit/lsrariscv64.cpp index a2a6152309f121..41fb0900bdef03 100644 --- a/src/coreclr/jit/lsrariscv64.cpp +++ b/src/coreclr/jit/lsrariscv64.cpp @@ -435,35 +435,8 @@ int LinearScan::BuildNode(GenTree* tree) break; case GT_EQ: - case GT_NE: case GT_LT: 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; From 7f9d38e81737d4bd2ce58117fdb6bf0654e8913e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 17 Jul 2025 17:58:48 +0200 Subject: [PATCH 04/36] a - C ---> a + (-C), also contain zero as first argument --- src/coreclr/jit/codegenriscv64.cpp | 9 +++++---- src/coreclr/jit/lower.cpp | 22 ++++++++++++--------- src/coreclr/jit/lowerriscv64.cpp | 3 +++ src/tests/JIT/Directed/compare/cmp_int32.cs | 18 +++++++++++++++++ 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 6246214dbda338..748348560177a2 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3122,11 +3122,10 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) assert(targetReg != REG_NA); assert(!tree->TypeIs(TYP_VOID)); - assert(!op1->isContainedIntOrIImmed()); if (varTypeIsFloating(op1Type)) { - assert(!op2->isContainedIntOrIImmed()); + assert(!op1->isContainedIntOrIImmed() && !op2->isContainedIntOrIImmed()); assert(op1->TypeIs(op2->TypeGet())); noway_assert((tree->gtFlags & GTF_RELOP_NAN_UN) == 0); @@ -3150,6 +3149,7 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) else { assert(tree->OperIs(GT_LT)); + assert(op1->isContainedIntOrIImmed() != op2->isContainedIntOrIImmed()); if (op2->isContainedIntOrIImmed()) { instruction slti = tree->IsUnsigned() ? INS_sltiu : INS_slti; @@ -3157,8 +3157,9 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) } else { - instruction slt = tree->IsUnsigned() ? INS_sltu : INS_slt; - emit->emitIns_R_R_R(slt, EA_PTRSIZE, targetReg, op1->GetRegNum(), op2->GetRegNum()); + instruction slt = tree->IsUnsigned() ? INS_sltu : INS_slt; + regNumber reg1 = op1->isContainedIntOrIImmed() ? REG_ZERO : op1->GetRegNum(); + emit->emitIns_R_R_R(slt, EA_PTRSIZE, targetReg, reg1, op2->GetRegNum()); } } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d76aa389cf12d3..77d651ce6eb103 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4472,12 +4472,16 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) if (!right->IsIntegralConst(0)) { // a == b ---> (a - b) == 0 - - // We may be comparing GC types, the type of the operands is GC but the type of the difference is - // not. The objects may be relocated but it's OK as the diff is used to only check against zero. - var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_LONG; - left = comp->gtNewOperNode(GT_SUB, type, left, right); - right = comp->gtNewZeroConNode(type); + var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_I_IMPL; + genTreeOps oper = GT_SUB; + if (right->IsIntegralConst() && !right->IsIntegralConst(SSIZE_T_MIN)) + { + // a - C ---> a + (-C) + oper = GT_ADD; + right->AsIntConCommon()->SetIntegralValue(-right->AsIntConCommon()->IntegralValue()); + } + left = comp->gtNewOperNode(oper, type, left, right); + right = comp->gtNewZeroConNode(type); BlockRange().InsertBefore(cmp, left); BlockRange().InsertBefore(cmp, right); ContainCheckBinary(left->AsOp()); @@ -4488,7 +4492,7 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) } assert(!cmp->OperIs(GT_EQ, GT_NE)); - if (right->IsIntegralConst() && cmp->OperIs(GT_LE, GT_GT)) + if (right->IsIntegralConst() && !right->IsIntegralConst(0) && cmp->OperIs(GT_LE, GT_GT)) { // a <= C ---> a < C+1 if (cmp->IsUnsigned() && right->IsIntegralConst(SIZE_T_MAX)) @@ -4518,12 +4522,12 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) // Integer comparisons come only in full-register variants so sign-extend operands as needed if (genActualTypeIsInt(left) && !left->OperIs(GT_ADD, GT_SUB, GT_CNS_INT)) { - left = comp->gtNewCastNode(TYP_INT, left, false, TYP_INT); + left = comp->gtNewCastNode(TYP_I_IMPL, left, false, TYP_I_IMPL); BlockRange().InsertAfter(left->gtGetOp1(), left); } if (genActualTypeIsInt(right) && !right->OperIs(GT_ADD, GT_SUB, GT_CNS_INT)) { - right = comp->gtNewCastNode(TYP_INT, right, false, TYP_INT); + right = comp->gtNewCastNode(TYP_I_IMPL, right, false, TYP_I_IMPL); BlockRange().InsertAfter(right->gtGetOp1(), right); } } diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 83f1400a384f90..9fbc1c46c5b096 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -1167,6 +1167,9 @@ void Lowering::ContainCheckCast(GenTreeCast* node) // void Lowering::ContainCheckCompare(GenTreeOp* cmp) { + if (cmp->gtOp1->IsIntegralConst(0)) + cmp->gtOp1->SetContained(); + CheckImmedAndMakeContained(cmp, cmp->gtOp2); } diff --git a/src/tests/JIT/Directed/compare/cmp_int32.cs b/src/tests/JIT/Directed/compare/cmp_int32.cs index 7bb555b7c2e635..e1ca0c79b8aea3 100644 --- a/src/tests/JIT/Directed/compare/cmp_int32.cs +++ b/src/tests/JIT/Directed/compare/cmp_int32.cs @@ -94,6 +94,12 @@ public static class CompareTestInt [MethodImplAttribute(MethodImplOptions.NoInlining)] public static bool NeMinus2048((int, float) x) => (x.Item1 != -2048); + [MethodImplAttribute(MethodImplOptions.NoInlining)] + public static bool EqMin((int, float) x) => (x.Item1 == int.MinValue); + + [MethodImplAttribute(MethodImplOptions.NoInlining)] + public static bool NeMin((int, float) x) => (x.Item1 != int.MinValue); + [Fact] public static void Test() { @@ -130,6 +136,9 @@ public static void Test() args = new object[] {(-2048, 0f)}; Assert.True((bool)type.GetMethod("EqMinus2048").Invoke(null, args)); Assert.False((bool)type.GetMethod("NeMinus2048").Invoke(null, args)); + args = new object[] {(int.MinValue, 0f)}; + Assert.True((bool)type.GetMethod("EqMin").Invoke(null, args)); + Assert.False((bool)type.GetMethod("NeMin").Invoke(null, args)); } } @@ -217,6 +226,12 @@ public static class CompareTestUint [MethodImplAttribute(MethodImplOptions.NoInlining)] public static bool NeMinus2048((uint, float) x) => (x.Item1 != 0xFFFF_F800u); + [MethodImplAttribute(MethodImplOptions.NoInlining)] + public static bool EqMax((uint, float) x) => (x.Item1 == uint.MaxValue); + + [MethodImplAttribute(MethodImplOptions.NoInlining)] + public static bool NeMax((uint, float) x) => (x.Item1 != uint.MaxValue); + [Fact] public static void Test() { @@ -253,5 +268,8 @@ public static void Test() args = new object[] {(0xFFFF_F800u, 0f)}; Assert.True((bool)type.GetMethod("EqMinus2048").Invoke(null, args)); Assert.False((bool)type.GetMethod("NeMinus2048").Invoke(null, args)); + args = new object[] {(uint.MaxValue, 0f)}; + Assert.True((bool)type.GetMethod("EqMax").Invoke(null, args)); + Assert.False((bool)type.GetMethod("NeMax").Invoke(null, args)); } } From b56a7fd3f2f770cd601733bbd14651e247270d7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 17 Jul 2025 23:22:15 +0200 Subject: [PATCH 05/36] Remove other ops from IsContainableImmed --- src/coreclr/jit/codegenriscv64.cpp | 5 ++--- src/coreclr/jit/lowerriscv64.cpp | 9 --------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 748348560177a2..25caa75ec59466 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3114,7 +3114,7 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) assert(!op1->isUsedFromMemory()); assert(!op2->isUsedFromMemory()); - emitAttr cmpSize = EA_ATTR(genTypeSize(op1Type)); + emitAttr cmpSize = EA_SIZE(genTypeSize(op1Type)); assert(cmpSize == EA_4BYTE || cmpSize == EA_8BYTE); emitter* emit = GetEmitter(); @@ -3148,8 +3148,7 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) } else { - assert(tree->OperIs(GT_LT)); - assert(op1->isContainedIntOrIImmed() != op2->isContainedIntOrIImmed()); + noway_assert(tree->OperIs(GT_LT)); if (op2->isContainedIntOrIImmed()) { instruction slti = tree->IsUnsigned() ? INS_sltiu : INS_slti; diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 9fbc1c46c5b096..168075642ded4e 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -64,16 +64,7 @@ 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: From 9434fc1b420595856763aa6966fa945006c791b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 18 Jul 2025 11:39:03 +0200 Subject: [PATCH 06/36] Cleanup LE|GE transformations --- src/coreclr/jit/lower.cpp | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 77d651ce6eb103..a1ce9104ad3433 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4490,27 +4490,31 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) cmp->SetOperRaw(cmp->OperIs(GT_EQ) ? GT_LE : GT_GT); cmp->SetUnsigned(); } - assert(!cmp->OperIs(GT_EQ, GT_NE)); - if (right->IsIntegralConst() && !right->IsIntegralConst(0) && cmp->OperIs(GT_LE, GT_GT)) + if (cmp->OperIs(GT_LE, GT_GE)) { - // a <= C ---> a < C+1 - if (cmp->IsUnsigned() && right->IsIntegralConst(SIZE_T_MAX)) + if (right->IsIntegralConst()) { - // (max+1) wraps to 0 so unsigned (a < max+1) would be always false instead of true - BlockRange().Remove(left); - BlockRange().Remove(right); - cmp->BashToConst(cmp->OperIs(GT_LE) ? 1 : 0); - return cmp->gtNext; + // a <= C ---> a < C+1 + INT64 value = right->AsIntConCommon()->IntegralValue(); + if (cmp->IsUnsigned() && + ((cmp->OperIs(GT_LE) && value == SIZE_T_MAX) || (cmp->OperIs(GT_GE) && value == 0))) + { + BlockRange().Remove(left); + BlockRange().Remove(right); + cmp->BashToConst(1); + return cmp->gtNext; + } + right->AsIntConCommon()->SetIntegralValue(cmp->OperIs(GT_LE) ? value + 1 : value - 1); + cmp->SetOperRaw(cmp->OperIs(GT_LE) ? GT_LT : GT_GT); + } + else + { + // a <= b ---> !(a > b) + isReversed = true; + cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); } - INT64 value = right->AsIntConCommon()->IntegralValue(); - right->AsIntConCommon()->SetIntegralValue(value + 1); - cmp->SetOperRaw(cmp->OperIs(GT_LE) ? GT_LT : GT_GE); - } - // a <= b ---> !(a > b) - isReversed = cmp->OperIs(GT_LE, GT_GE); - if (isReversed) - cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); + } if (cmp->OperIs(GT_GT)) { From 0296573e06f73d17e3978b4019264a52a097a083 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 18 Jul 2025 11:39:39 +0200 Subject: [PATCH 07/36] Factor out sign-extension --- src/coreclr/jit/lower.cpp | 13 ++----------- src/coreclr/jit/lower.h | 1 + src/coreclr/jit/lowerriscv64.cpp | 20 ++++++++++++++++++++ 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a1ce9104ad3433..f061bb55e78544 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4523,17 +4523,8 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) } assert(cmp->OperIs(GT_LT)); - // Integer comparisons come only in full-register variants so sign-extend operands as needed - if (genActualTypeIsInt(left) && !left->OperIs(GT_ADD, GT_SUB, GT_CNS_INT)) - { - left = comp->gtNewCastNode(TYP_I_IMPL, left, false, TYP_I_IMPL); - BlockRange().InsertAfter(left->gtGetOp1(), left); - } - if (genActualTypeIsInt(right) && !right->OperIs(GT_ADD, GT_SUB, GT_CNS_INT)) - { - right = comp->gtNewCastNode(TYP_I_IMPL, right, false, TYP_I_IMPL); - BlockRange().InsertAfter(right->gtGetOp1(), right); - } + SignExtendIfNecessary(&left); + SignExtendIfNecessary(&right); } if (isReversed) { diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index dd4d2d55578f56..172e990dc6891c 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -161,6 +161,7 @@ class Lowering final : public Phase GenTree* LowerJTrue(GenTreeOp* jtrue); #ifdef TARGET_RISCV64 bool LowerAndReverseFloatingCompare(GenTree* cmp); + void SignExtendIfNecessary(GenTree** arg); #endif GenTree* LowerSelect(GenTreeConditional* cond); bool TryLowerConditionToFlagsNode(GenTree* parent, diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 168075642ded4e..f188bf4c11ff2a 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -204,6 +204,26 @@ bool Lowering::LowerAndReverseFloatingCompare(GenTree* cmp) return isUnordered; } +//------------------------------------------------------------------------ +// 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_CNS_INT)) + return; + + *arg = comp->gtNewCastNode(TYP_I_IMPL, *arg, false, TYP_I_IMPL); + BlockRange().InsertAfter((*arg)->gtGetOp1(), *arg); +} + //------------------------------------------------------------------------ // LowerBinaryArithmetic: lowers the given binary arithmetic node. // From 1bae2d77b8b0078b1ce7885e33a3f2e2b7e027e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 18 Jul 2025 17:08:42 +0200 Subject: [PATCH 08/36] Factor out LowerAndReverseIntegerCompare --- src/coreclr/jit/lower.cpp | 97 +++++--------------------------- src/coreclr/jit/lower.h | 1 + src/coreclr/jit/lowerriscv64.cpp | 84 +++++++++++++++++++++++++-- 3 files changed, 94 insertions(+), 88 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index f061bb55e78544..7452efe0b7c173 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4448,92 +4448,21 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) } } #elif defined(TARGET_RISCV64) - // Note: branch instructions are fused with a full range of integer comparisons. However, comparisons which save the - // result to a register are covered with fewer instructions: - // * for integer: "<" (with unsigned and immediate variants), - // * for floating: "<", "<=", "==" (ordered only, register only), - // so the rest is achieved through various transformations. - GenTree*& left = cmp->AsOp()->gtOp1; - GenTree*& right = cmp->AsOp()->gtOp2; - LIR::Use cmpUse; - if (BlockRange().TryGetUse(cmp, &cmpUse) && !cmpUse.User()->OperIs(GT_JTRUE)) - { - bool isReversed = false; - if (varTypeIsFloating(left)) - { - // a CMP b (unordered) ---> !(a reversedCMP b (ordered)) - isReversed = LowerAndReverseFloatingCompare(cmp); - } - else - { - assert(varTypeUsesIntReg(left)); - if (cmp->OperIs(GT_EQ, GT_NE)) - { - if (!right->IsIntegralConst(0)) - { - // a == b ---> (a - b) == 0 - var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_I_IMPL; - genTreeOps oper = GT_SUB; - if (right->IsIntegralConst() && !right->IsIntegralConst(SSIZE_T_MIN)) - { - // a - C ---> a + (-C) - oper = GT_ADD; - right->AsIntConCommon()->SetIntegralValue(-right->AsIntConCommon()->IntegralValue()); - } - left = comp->gtNewOperNode(oper, type, left, right); - right = comp->gtNewZeroConNode(type); - BlockRange().InsertBefore(cmp, left); - BlockRange().InsertBefore(cmp, right); - ContainCheckBinary(left->AsOp()); - } - // a == 0 ---> a <= 0 (unsigned) - cmp->SetOperRaw(cmp->OperIs(GT_EQ) ? GT_LE : GT_GT); - cmp->SetUnsigned(); - } - - if (cmp->OperIs(GT_LE, GT_GE)) - { - if (right->IsIntegralConst()) - { - // a <= C ---> a < C+1 - INT64 value = right->AsIntConCommon()->IntegralValue(); - if (cmp->IsUnsigned() && - ((cmp->OperIs(GT_LE) && value == SIZE_T_MAX) || (cmp->OperIs(GT_GE) && value == 0))) - { - BlockRange().Remove(left); - BlockRange().Remove(right); - cmp->BashToConst(1); - return cmp->gtNext; - } - right->AsIntConCommon()->SetIntegralValue(cmp->OperIs(GT_LE) ? value + 1 : value - 1); - cmp->SetOperRaw(cmp->OperIs(GT_LE) ? GT_LT : GT_GT); - } - else - { - // a <= b ---> !(a > b) - isReversed = true; - cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); - } - } - - if (cmp->OperIs(GT_GT)) - { - cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); - std::swap(left, right); - } - assert(cmp->OperIs(GT_LT)); + // Branch instructions are fused with a full range of integer comparisons, they will be lowered in LowerJTrue + LIR::Use cmpUse; + if (!BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE)) + return cmp->gtNext; - SignExtendIfNecessary(&left); - SignExtendIfNecessary(&right); - } - if (isReversed) - { - GenTree* notOp = comp->gtNewOperNode(GT_NOT, cmp->gtType, cmp); - BlockRange().InsertAfter(cmp, notOp); - cmpUse.ReplaceWith(notOp); - } + bool isReversed = + varTypeIsFloating(cmp->gtGetOp1()) ? LowerAndReverseFloatingCompare(cmp) : LowerAndReverseIntegerCompare(cmp); + if (isReversed) + { + GenTree* notOp = comp->gtNewOperNode(GT_NOT, cmp->gtType, cmp); + BlockRange().InsertAfter(cmp, notOp); + cmpUse.ReplaceWith(notOp); } - + if (!cmp->OperIsCmpCompare()) // comparison was optimized out to some other node + return cmp->gtNext; #endif // TARGET_RISCV64 ContainCheckCompare(cmp->AsOp()); diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 172e990dc6891c..3ee71f05feb365 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -161,6 +161,7 @@ class Lowering final : public Phase GenTree* LowerJTrue(GenTreeOp* jtrue); #ifdef TARGET_RISCV64 bool LowerAndReverseFloatingCompare(GenTree* cmp); + bool LowerAndReverseIntegerCompare(GenTree* cmp); void SignExtendIfNecessary(GenTree** arg); #endif GenTree* LowerSelect(GenTreeConditional* cond); diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index f188bf4c11ff2a..bb8a30b072d0a7 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -175,8 +175,8 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) } //------------------------------------------------------------------------ -// LowerAndReverseFloatingCompare: lowers and reverses a floating-point comparison so that its oper matches the -// available instructions +// LowerAndReverseFloatingCompare: lowers and reverses a floating-point comparison so that it matches the +// available instructions: "<", "<=", "==" (ordered only, register only) // // Arguments: // cmp - the floating-point comparison to lower @@ -190,8 +190,7 @@ bool Lowering::LowerAndReverseFloatingCompare(GenTree* cmp) bool isUnordered = (cmp->gtFlags & GTF_RELOP_NAN_UN) != 0; if (isUnordered) { - // Unordered floating-point comparisons are achieved by neg'ing the ordered counterparts. Avoid that by - // reversing the FP comparison and communicating to the caller to reverse its use appropriately. + // a CMP b (unordered) ---> !(a reversedCMP b (ordered)) cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); cmp->gtFlags &= ~GTF_RELOP_NAN_UN; } @@ -204,6 +203,83 @@ bool Lowering::LowerAndReverseFloatingCompare(GenTree* cmp) return isUnordered; } +//------------------------------------------------------------------------ +// LowerAndReverseIntegerCompare: lowers and reverses an integer comparison so that it matches the available +// instructions: only "<" (with unsigned and immediate variants) +// +// Arguments: +// cmp - the integer comparison to lower +// +// Returns: +// Whether the comparison was reversed. +// +bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) +{ + GenTree*& left = cmp->AsOp()->gtOp1; + GenTree*& right = cmp->AsOp()->gtOp2; + assert(cmp->OperIsCmpCompare() && varTypeUsesIntReg(left)); + bool isReversed = false; + if (cmp->OperIs(GT_EQ, GT_NE)) + { + if (!right->IsIntegralConst(0)) + { + // a == b ---> (a - b) == 0 + var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_I_IMPL; + genTreeOps oper = GT_SUB; + if (right->IsIntegralConst() && !right->IsIntegralConst(SSIZE_T_MIN)) + { + // a - C ---> a + (-C) + oper = GT_ADD; + right->AsIntConCommon()->SetIntegralValue(-right->AsIntConCommon()->IntegralValue()); + } + left = comp->gtNewOperNode(oper, type, left, right); + right = comp->gtNewZeroConNode(type); + BlockRange().InsertBefore(cmp, left); + BlockRange().InsertBefore(cmp, right); + ContainCheckBinary(left->AsOp()); + } + // a == 0 ---> a <= 0 (unsigned) + cmp->SetOperRaw(cmp->OperIs(GT_EQ) ? GT_LE : GT_GT); + cmp->SetUnsigned(); + } + + if (cmp->OperIs(GT_LE, GT_GE)) + { + if (right->IsIntegralConst()) + { + // a <= C ---> a < C+1 + INT64 value = right->AsIntConCommon()->IntegralValue(); + if (cmp->IsUnsigned() && + ((cmp->OperIs(GT_LE) && value == SIZE_T_MAX) || (cmp->OperIs(GT_GE) && value == 0))) + { + BlockRange().Remove(left); + BlockRange().Remove(right); + cmp->BashToConst(1); + return false; + } + right->AsIntConCommon()->SetIntegralValue(cmp->OperIs(GT_LE) ? value + 1 : value - 1); + cmp->SetOperRaw(cmp->OperIs(GT_LE) ? GT_LT : GT_GT); + } + else + { + // a <= b ---> !(a > b) + isReversed = true; + cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); + } + } + + if (cmp->OperIs(GT_GT)) + { + cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); + std::swap(left, right); + } + assert(cmp->OperIs(GT_LT)); + + SignExtendIfNecessary(&left); + SignExtendIfNecessary(&right); + return isReversed; +} + //------------------------------------------------------------------------ // SignExtendIfNecessary: inserts a 32-bit sign extension unless the argument is full-register or is known to be // implemented with a sign-extending instruction. From a5013918fd26f880fc5495948ef0dbd1545fcf39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Mon, 21 Jul 2025 13:13:34 +0200 Subject: [PATCH 09/36] Move compare-and-branch logic to lowering --- src/coreclr/jit/codegenriscv64.cpp | 193 +++++------------------------ src/coreclr/jit/lower.cpp | 2 +- src/coreclr/jit/lowerriscv64.cpp | 66 +++++----- src/coreclr/jit/lsrariscv64.cpp | 5 - 4 files changed, 63 insertions(+), 203 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 25caa75ec59466..2060cf84030434 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3182,174 +3182,47 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) assert(compiler->compCurBB->KindIs(BBJ_COND)); assert(tree->OperIs(GT_JCMP)); - assert(!varTypeIsFloating(tree)); assert(tree->TypeIs(TYP_VOID)); assert(tree->GetRegNum() == REG_NA); - GenTree* op1 = tree->gtGetOp1(); - GenTree* op2 = tree->gtGetOp2(); - assert(!op1->isUsedFromMemory()); - assert(!op2->isUsedFromMemory()); - assert(!op1->isContainedIntOrIImmed()); - - var_types op1Type = genActualType(op1->TypeGet()); - var_types op2Type = genActualType(op2->TypeGet()); - assert(genTypeSize(op1Type) == genTypeSize(op2Type)); - genConsumeOperands(tree); - - emitter* emit = GetEmitter(); - instruction ins = INS_invalid; - int regs = 0; - - GenCondition cond = tree->gtCondition; - - emitAttr cmpSize = EA_ATTR(genTypeSize(op1Type)); - regNumber regOp1 = op1->GetRegNum(); - - if (op2->isContainedIntOrIImmed()) + instruction ins = INS_invalid; + switch (tree->gtCondition.GetCode()) { - ssize_t imm = op2->AsIntCon()->gtIconVal; - if (imm) - { - assert(regOp1 != REG_R0); - switch (cmpSize) - { - case EA_4BYTE: - { - regNumber tmpRegOp1 = rsGetRsvdReg(); - assert(regOp1 != tmpRegOp1); - imm = static_cast(imm); - emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp1, regOp1); - regOp1 = tmpRegOp1; - break; - } - case EA_8BYTE: - break; - default: - unreached(); - } - - GenTreeIntCon* con = op2->AsIntCon(); - - emitAttr attr = emitActualTypeSize(op2Type); - // TODO-CQ: Currently we cannot do this for all handles because of - // https://github.com/dotnet/runtime/issues/60712 - if (con->ImmedValNeedsReloc(compiler)) - { - attr = EA_SET_FLG(attr, EA_CNS_RELOC_FLG); - } - - if (op2Type == TYP_BYREF) - { - attr = EA_SET_FLG(attr, EA_BYREF_FLG); - } - - instGen_Set_Reg_To_Imm(attr, REG_RA, imm, - INS_FLAGS_DONT_CARE DEBUGARG(con->gtTargetHandle) DEBUGARG(con->gtFlags)); - regSet.verifyRegUsed(REG_RA); - regs = (int)REG_RA << 5; - } - else - { - if (cmpSize == EA_4BYTE) - { - regNumber tmpRegOp1 = rsGetRsvdReg(); - assert(regOp1 != tmpRegOp1); - emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp1, regOp1); - regOp1 = tmpRegOp1; - } - } - - switch (cond.GetCode()) - { - case GenCondition::EQ: - regs |= ((int)regOp1); - ins = INS_beq; - break; - case GenCondition::NE: - regs |= ((int)regOp1); - ins = INS_bne; - break; - case GenCondition::UGE: - case GenCondition::SGE: - regs |= ((int)regOp1); - ins = cond.IsUnsigned() ? INS_bgeu : INS_bge; - break; - case GenCondition::UGT: - case GenCondition::SGT: - regs = imm ? ((((int)regOp1) << 5) | (int)REG_RA) : (((int)regOp1) << 5); - ins = cond.IsUnsigned() ? INS_bltu : INS_blt; - break; - case GenCondition::ULT: - case GenCondition::SLT: - regs |= ((int)regOp1); - ins = cond.IsUnsigned() ? INS_bltu : INS_blt; - break; - case GenCondition::ULE: - case GenCondition::SLE: - regs = imm ? ((((int)regOp1) << 5) | (int)REG_RA) : (((int)regOp1) << 5); - ins = cond.IsUnsigned() ? INS_bgeu : INS_bge; - break; - default: - NO_WAY("unexpected condition type"); - break; - } + case GenCondition::EQ: + ins = INS_beq; + break; + case GenCondition::NE: + ins = INS_bne; + break; + case GenCondition::SGE: + ins = INS_bge; + break; + case GenCondition::UGE: + ins = INS_bgeu; + break; + case GenCondition::SLT: + ins = INS_blt; + break; + case GenCondition::ULT: + ins = INS_bltu; + break; + default: + NO_WAY("unexpected condition type-regs"); + break; } - else - { - regNumber regOp2 = op2->GetRegNum(); - if (cmpSize == EA_4BYTE) - { - regNumber tmpRegOp1 = REG_RA; - regNumber tmpRegOp2 = rsGetRsvdReg(); - assert(regOp1 != tmpRegOp2); - assert(regOp2 != tmpRegOp2); - emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp1, regOp1); - emit->emitIns_R_R(INS_sext_w, EA_8BYTE, tmpRegOp2, regOp2); - regOp1 = tmpRegOp1; - regOp2 = tmpRegOp2; - } - switch (cond.GetCode()) - { - case GenCondition::EQ: - regs = (((int)regOp1) << 5) | (int)regOp2; - ins = INS_beq; - break; - case GenCondition::NE: - regs = (((int)regOp1) << 5) | (int)regOp2; - ins = INS_bne; - break; - case GenCondition::UGE: - case GenCondition::SGE: - regs = ((int)regOp1 | ((int)regOp2 << 5)); - ins = cond.IsUnsigned() ? INS_bgeu : INS_bge; - break; - case GenCondition::UGT: - case GenCondition::SGT: - regs = (((int)regOp1) << 5) | (int)regOp2; - ins = cond.IsUnsigned() ? INS_bltu : INS_blt; - break; - case GenCondition::ULT: - case GenCondition::SLT: - regs = ((int)regOp1 | ((int)regOp2 << 5)); - ins = cond.IsUnsigned() ? INS_bltu : INS_blt; - break; - case GenCondition::ULE: - case GenCondition::SLE: - regs = (((int)regOp1) << 5) | (int)regOp2; - ins = cond.IsUnsigned() ? INS_bgeu : INS_bge; - break; - default: - NO_WAY("unexpected condition type-regs"); - break; - } - } - assert(ins != INS_invalid); + GenTree* op1 = tree->gtGetOp1(); + GenTree* op2 = tree->gtGetOp2(); + assert(!op1->isUsedFromMemory()); + assert(!op2->isUsedFromMemory()); + assert(op1->isContained() == op1->IsIntegralConst(0)); + assert(op2->isContained() == op2->IsIntegralConst(0)); + regNumber regOp1 = op1->isContained() ? REG_ZERO : op1->GetRegNum(); + regNumber regOp2 = op2->isContained() ? REG_ZERO : op2->GetRegNum(); + int regs = (int)regOp1 | (((int)regOp2) << 5); assert(regs != 0); - - emit->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); // 5-bits; + GetEmitter()->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); // If we cannot fall into the false target, emit a jump to it BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget(); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 7452efe0b7c173..eaa295396ad521 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4448,7 +4448,7 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) } } #elif defined(TARGET_RISCV64) - // Branch instructions are fused with a full range of integer comparisons, they will be lowered in LowerJTrue + // Branches will be lowered in LowerJTrue LIR::Use cmpUse; if (!BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE)) return cmp->gtNext; diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index bb8a30b072d0a7..5c5f8504e5e2ee 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -70,9 +70,8 @@ bool Lowering::IsContainableImmed(GenTree* parentNode, GenTree* childNode) const case GT_OR: case GT_XOR: return emitter::isValidSimm12(immVal); - case GT_JCMP: - return true; + case GT_JCMP: case GT_CMPXCHG: case GT_XORR: case GT_XAND: @@ -124,54 +123,47 @@ GenTree* Lowering::LowerMul(GenTreeOp* mul) // GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) { - GenTree* op = jtrue->gtGetOp1(); - GenCondition cond; - GenTree* cmpOp1; - GenTree* cmpOp2; + GenTree* cmp = jtrue->gtGetOp1(); + assert(!cmp->OperIsCompare() || cmp->OperIsCmpCompare()); // We do not expect any other relops on RISCV64 - assert(!op->OperIsCompare() || op->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); + // Branch instructions are fused with a full range of integer comparisons so just remove the comparison + jcmp->gtCondition = GenCondition::FromIntegralRelop(cmp); + jcmp->gtOp1 = cmp->gtGetOp1(); + jcmp->gtOp2 = cmp->gtGetOp2(); + if (cmp->OperIs(GT_GT, GT_LE)) + { + // ">" and "<=" are achieved by swapping inputs for "<" and ">=" + jcmp->gtCondition = GenCondition::Swap(jcmp->gtCondition); + std::swap(jcmp->gtOp1, jcmp->gtOp2); + } + BlockRange().Remove(cmp); } else { + // branch if (cond) ---> branch if (cond != 0) GenCondition::Code code = GenCondition::NE; - if (op->OperIsCompare() && varTypeIsFloating(op->gtGetOp1())) + if (cmp->OperIsCompare() && varTypeIsFloating(cmp->gtGetOp1())) { - if (LowerAndReverseFloatingCompare(op)) + if (LowerAndReverseFloatingCompare(cmp)) code = GenCondition::EQ; } - cond = GenCondition(code); - - cmpOp1 = op; - cmpOp2 = comp->gtNewZeroConNode(cmpOp1->TypeGet()); - - BlockRange().InsertBefore(jtrue, cmpOp2); - - // Fall through and turn this into a JCMP(op1, 0, NE). + jcmp->gtCondition = GenCondition(code); + jcmp->gtOp1 = cmp; + jcmp->gtOp2 = comp->gtNewZeroConNode(cmp->TypeGet()); + BlockRange().InsertBefore(jcmp, jcmp->gtOp2); } - // 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; - - if (cmpOp2->IsCnsIntOrI()) + for (GenTree** op : {&jcmp->gtOp1, &jcmp->gtOp2}) { - cmpOp2->SetContained(); + SignExtendIfNecessary(op); + CheckImmedAndMakeContained(jcmp, *op); } - - return jtrue->gtNext; + return jcmp->gtNext; } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/lsrariscv64.cpp b/src/coreclr/jit/lsrariscv64.cpp index 41fb0900bdef03..d5e4adec2929b1 100644 --- a/src/coreclr/jit/lsrariscv64.cpp +++ b/src/coreclr/jit/lsrariscv64.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); From 2a3543e189748a29c338dc6defc2788c5a0e4786 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 24 Jul 2025 09:10:27 +0200 Subject: [PATCH 10/36] Fix perf regressions in int comparisons when stored to a register --- src/coreclr/jit/lowerriscv64.cpp | 49 +++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 5c5f8504e5e2ee..43d93617b8d58d 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -218,11 +218,23 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) // a == b ---> (a - b) == 0 var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_I_IMPL; genTreeOps oper = GT_SUB; - if (right->IsIntegralConst() && !right->IsIntegralConst(SSIZE_T_MIN)) + if (right->IsIntegralConst()) { - // a - C ---> a + (-C) - oper = GT_ADD; - right->AsIntConCommon()->SetIntegralValue(-right->AsIntConCommon()->IntegralValue()); + 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 (value != minVal) + { + // a - C ---> a + (-C) + oper = GT_ADD; + right->AsIntConCommon()->SetIntegralValue(-value); + } } left = comp->gtNewOperNode(oper, type, left, right); right = comp->gtNewZeroConNode(type); @@ -230,7 +242,7 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) BlockRange().InsertBefore(cmp, right); ContainCheckBinary(left->AsOp()); } - // a == 0 ---> a <= 0 (unsigned) + // a != 0 ---> a > 0 (unsigned) cmp->SetOperRaw(cmp->OperIs(GT_EQ) ? GT_LE : GT_GT); cmp->SetUnsigned(); } @@ -241,8 +253,21 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) { // a <= C ---> a < C+1 INT64 value = right->AsIntConCommon()->IntegralValue(); - if (cmp->IsUnsigned() && - ((cmp->OperIs(GT_LE) && value == SIZE_T_MAX) || (cmp->OperIs(GT_GE) && value == 0))) + + 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) { BlockRange().Remove(left); BlockRange().Remove(right); @@ -267,6 +292,16 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) } assert(cmp->OperIs(GT_LT)); + if (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 isReversed; + } + SignExtendIfNecessary(&left); SignExtendIfNecessary(&right); return isReversed; From bceb8fafe3c3f08bb3fb7e5d9794b3d7fcbeb912 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 24 Jul 2025 09:43:36 +0200 Subject: [PATCH 11/36] Negate via XOR 1, NOT can be ambigugous --- src/coreclr/jit/codegenriscv64.cpp | 3 +-- src/coreclr/jit/lower.cpp | 6 ++++-- src/coreclr/jit/lowerriscv64.cpp | 3 +-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 2060cf84030434..36bd80100ff5ae 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -1691,8 +1691,7 @@ void CodeGen::genCodeForNegNot(GenTree* tree) else if (tree->OperIs(GT_NOT)) { assert(!varTypeIsFloating(targetType)); - ssize_t mask = tree->gtGetOp1()->OperIsCmpCompare() ? 1 : -1; - GetEmitter()->emitIns_R_R_I(INS_xori, attr, targetReg, operandReg, mask); + GetEmitter()->emitIns_R_R_I(INS_xori, attr, targetReg, operandReg, -1); } genProduceReg(tree); diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index eaa295396ad521..8fc33b3d0cc396 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4457,8 +4457,10 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) varTypeIsFloating(cmp->gtGetOp1()) ? LowerAndReverseFloatingCompare(cmp) : LowerAndReverseIntegerCompare(cmp); if (isReversed) { - GenTree* notOp = comp->gtNewOperNode(GT_NOT, cmp->gtType, cmp); - BlockRange().InsertAfter(cmp, notOp); + GenTree* one = comp->gtNewIconNode(1); + GenTree* notOp = comp->gtNewOperNode(GT_XOR, cmp->gtType, cmp, one); + BlockRange().InsertAfter(cmp, one, notOp); + one->SetContained(); cmpUse.ReplaceWith(notOp); } if (!cmp->OperIsCmpCompare()) // comparison was optimized out to some other node diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 43d93617b8d58d..372a32c35e49e5 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -238,8 +238,7 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) } left = comp->gtNewOperNode(oper, type, left, right); right = comp->gtNewZeroConNode(type); - BlockRange().InsertBefore(cmp, left); - BlockRange().InsertBefore(cmp, right); + BlockRange().InsertBefore(cmp, left, right); ContainCheckBinary(left->AsOp()); } // a != 0 ---> a > 0 (unsigned) From 2dffd3131d439cded556fbaa383cfb898cf35edc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 24 Jul 2025 12:28:43 +0200 Subject: [PATCH 12/36] Add more known 32-bit extending nodes --- src/coreclr/jit/lowerriscv64.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 372a32c35e49e5..89af19151aa449 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -319,7 +319,10 @@ void Lowering::SignExtendIfNecessary(GenTree** arg) if (!genActualTypeIsInt(*arg)) return; - if ((*arg)->OperIs(GT_ADD, GT_SUB, GT_CNS_INT)) + if ((*arg)->OperIs(GT_ADD, GT_SUB, GT_MUL, GT_MOD, GT_UMOD, GT_DIV, GT_UDIV, GT_CNS_INT)) + return; + + if ((*arg)->OperIsShiftOrRotate() || (*arg)->OperIsCmpCompare() || (*arg)->OperIsAtomicOp()) return; *arg = comp->gtNewCastNode(TYP_I_IMPL, *arg, false, TYP_I_IMPL); From 8160b47d7836caaf5b2c8b889043d796bbb33155 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 25 Jul 2025 12:13:07 +0200 Subject: [PATCH 13/36] handle 1st branch arg contained in BBJ_COND lsra --- src/coreclr/jit/lsra.cpp | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 22dea851621b87..270aa90f96385c 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -9040,23 +9040,29 @@ 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() + : (!lastNode->gtGetOp1()->isContained() || !lastNode->gtGetOp2()->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)) From dd8615dedebf472da748f7c57c1a07c9b5ace3bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Wed, 30 Jul 2025 15:54:54 +0200 Subject: [PATCH 14/36] Don't comparison transformations which include altering the constant value when the value is relocatable --- src/coreclr/jit/lowerriscv64.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 89af19151aa449..ec531b094a8cae 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -163,6 +163,8 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) SignExtendIfNecessary(op); CheckImmedAndMakeContained(jcmp, *op); } + assert(!jcmp->gtOp1->isContained() || !jcmp->gtOp2->isContained()); + return jcmp->gtNext; } @@ -218,7 +220,7 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) // a == b ---> (a - b) == 0 var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_I_IMPL; genTreeOps oper = GT_SUB; - if (right->IsIntegralConst()) + if (right->IsIntegralConst() && !right->AsIntCon()->ImmedValNeedsReloc(comp)) { INT64 value = right->AsIntConCommon()->IntegralValue(); INT64 minVal = (type == TYP_INT) ? INT_MIN : SSIZE_T_MIN; @@ -248,7 +250,7 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) if (cmp->OperIs(GT_LE, GT_GE)) { - if (right->IsIntegralConst()) + if (right->IsIntegralConst() && !right->AsIntCon()->ImmedValNeedsReloc(comp)) { // a <= C ---> a < C+1 INT64 value = right->AsIntConCommon()->IntegralValue(); @@ -291,7 +293,7 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) } assert(cmp->OperIs(GT_LT)); - if (right->IsIntegralConst(0) && !cmp->IsUnsigned()) + if (right->IsIntegralConst(0) && !right->AsIntCon()->ImmedValNeedsReloc(comp) && !cmp->IsUnsigned()) { // a < 0 (signed) ---> shift the sign bit into the lowest bit cmp->SetOperRaw(GT_RSZ); @@ -1283,10 +1285,11 @@ void Lowering::ContainCheckCast(GenTreeCast* node) // void Lowering::ContainCheckCompare(GenTreeOp* cmp) { - if (cmp->gtOp1->IsIntegralConst(0)) - cmp->gtOp1->SetContained(); + if (cmp->gtOp1->IsIntegralConst(0) && !cmp->gtOp1->AsIntCon()->ImmedValNeedsReloc(comp)) + MakeSrcContained(cmp, cmp->gtOp1); CheckImmedAndMakeContained(cmp, cmp->gtOp2); + assert(!cmp->gtOp1->isContained() || !cmp->gtOp2->isContained()); } //------------------------------------------------------------------------ From cb9d11055900803f7222b81fef61315b865ccc25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 31 Jul 2025 11:40:14 +0200 Subject: [PATCH 15/36] LowerNode after converting UDIV into GE --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 8fc33b3d0cc396..1dac56c0b6be7b 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7712,7 +7712,7 @@ bool Lowering::LowerUnsignedDivOrMod(GenTreeOp* divMod) { divMod->ChangeOper(GT_GE); divMod->gtFlags |= GTF_UNSIGNED; - ContainCheckNode(divMod); + LowerNode(divMod); return true; } } From c8ed91cd3781e7bab0816c39622dbb5392c6761f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 31 Jul 2025 14:53:19 +0200 Subject: [PATCH 16/36] Don't zero-extend uint MUL, the natural state is to leave it sign-extended like mulw does --- src/coreclr/jit/emitriscv64.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/coreclr/jit/emitriscv64.cpp b/src/coreclr/jit/emitriscv64.cpp index bf11222800d09b..c454a362eb8a17 100644 --- a/src/coreclr/jit/emitriscv64.cpp +++ b/src/coreclr/jit/emitriscv64.cpp @@ -5250,15 +5250,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); From 3ca9be2a0f4bf2e413b013ef15bdf569f10085e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 31 Jul 2025 17:01:29 +0200 Subject: [PATCH 17/36] First argument of a comparison can also be contained -- when it's zero --- src/coreclr/jit/codegenriscv64.cpp | 12 ++++++------ src/coreclr/jit/emitriscv64.cpp | 2 +- src/coreclr/jit/lowerriscv64.cpp | 2 -- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 36bd80100ff5ae..e64ade4dc46bd1 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3148,15 +3148,16 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) else { noway_assert(tree->OperIs(GT_LT)); + assert(!op1->isContainedIntOrIImmed() || op1->IsIntegralConst(0)); + regNumber reg1 = op1->isContainedIntOrIImmed() ? REG_ZERO : op1->GetRegNum(); if (op2->isContainedIntOrIImmed()) { instruction slti = tree->IsUnsigned() ? INS_sltiu : INS_slti; - emit->emitIns_R_R_I(slti, EA_PTRSIZE, targetReg, op1->GetRegNum(), op2->AsIntCon()->gtIconVal); + emit->emitIns_R_R_I(slti, EA_PTRSIZE, targetReg, reg1, op2->AsIntCon()->gtIconVal); } else { - instruction slt = tree->IsUnsigned() ? INS_sltu : INS_slt; - regNumber reg1 = op1->isContainedIntOrIImmed() ? REG_ZERO : op1->GetRegNum(); + instruction slt = tree->IsUnsigned() ? INS_sltu : INS_slt; emit->emitIns_R_R_R(slt, EA_PTRSIZE, targetReg, reg1, op2->GetRegNum()); } } @@ -3215,12 +3216,11 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) GenTree* op2 = tree->gtGetOp2(); assert(!op1->isUsedFromMemory()); assert(!op2->isUsedFromMemory()); - assert(op1->isContained() == op1->IsIntegralConst(0)); - assert(op2->isContained() == op2->IsIntegralConst(0)); + assert(!op1->isContained() || op1->IsIntegralConst(0)); + assert(!op1->isContained() || op1->IsIntegralConst(0)); regNumber regOp1 = op1->isContained() ? REG_ZERO : op1->GetRegNum(); regNumber regOp2 = op2->isContained() ? REG_ZERO : op2->GetRegNum(); int regs = (int)regOp1 | (((int)regOp2) << 5); - assert(regs != 0); GetEmitter()->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); // If we cannot fall into the false target, emit a jump to it diff --git a/src/coreclr/jit/emitriscv64.cpp b/src/coreclr/jit/emitriscv64.cpp index c454a362eb8a17..71bb7b5f3e41d1 100644 --- a/src/coreclr/jit/emitriscv64.cpp +++ b/src/coreclr/jit/emitriscv64.cpp @@ -730,7 +730,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 diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index ec531b094a8cae..8a4cf453fa124f 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -163,7 +163,6 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) SignExtendIfNecessary(op); CheckImmedAndMakeContained(jcmp, *op); } - assert(!jcmp->gtOp1->isContained() || !jcmp->gtOp2->isContained()); return jcmp->gtNext; } @@ -1289,7 +1288,6 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) MakeSrcContained(cmp, cmp->gtOp1); CheckImmedAndMakeContained(cmp, cmp->gtOp2); - assert(!cmp->gtOp1->isContained() || !cmp->gtOp2->isContained()); } //------------------------------------------------------------------------ From 170906c3a7285b921166725cf2d5ceeb213b26b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 1 Aug 2025 10:49:52 +0200 Subject: [PATCH 18/36] cleanup --- src/coreclr/jit/lowerriscv64.cpp | 1 - src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs | 7 ------- 2 files changed, 8 deletions(-) diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 8a4cf453fa124f..e68c5a97678bca 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -131,7 +131,6 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) GenTreeOpCC* jcmp = jtrue->AsOpCC(); if (cmp->OperIsCompare() && !varTypeIsFloating(cmp->gtGetOp1())) { - // Branch instructions are fused with a full range of integer comparisons so just remove the comparison jcmp->gtCondition = GenCondition::FromIntegralRelop(cmp); jcmp->gtOp1 = cmp->gtGetOp1(); jcmp->gtOp2 = cmp->gtGetOp2(); diff --git a/src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs b/src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs index c2a63451083a1d..15c22ec83d8f2a 100644 --- a/src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs +++ b/src/tests/JIT/CodeGenBringUpTests/JTrueGeFP.cs @@ -24,12 +24,6 @@ public static int JTrueGeFP(float x) return returnValue; } - - [MethodImplAttribute(MethodImplOptions.NoInlining)] - public static bool JTrueGeFP_bool(float x) - { - return (x >= 2f); - } [Fact] public static int TestEntryPoint() @@ -40,7 +34,6 @@ public static int TestEntryPoint() if (JTrueGeFP(0f) != 2) returnValue = Fail; if (JTrueGeFP(1f) != 3) returnValue = Fail; if (JTrueGeFP(2f) != 4) returnValue = Fail; - if (!JTrueGeFP_bool(2f)) returnValue = Fail; return returnValue; From b038856704ebeddaf0d6442417767eb49e730839 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 1 Aug 2025 11:06:25 +0200 Subject: [PATCH 19/36] Fix branch asserts --- src/coreclr/jit/codegenriscv64.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index e64ade4dc46bd1..588c362d9cfbd1 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3208,7 +3208,7 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) ins = INS_bltu; break; default: - NO_WAY("unexpected condition type-regs"); + NO_WAY("unexpected branch condition"); break; } From 1e95bf984f045cf22c3b534cb564a3065c9cf845 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 1 Aug 2025 11:22:09 +0200 Subject: [PATCH 20/36] assert branch regs are general --- src/coreclr/jit/codegenriscv64.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 588c362d9cfbd1..9e0fda4744897d 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3217,10 +3217,11 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) assert(!op1->isUsedFromMemory()); assert(!op2->isUsedFromMemory()); assert(!op1->isContained() || op1->IsIntegralConst(0)); - assert(!op1->isContained() || op1->IsIntegralConst(0)); - regNumber regOp1 = op1->isContained() ? REG_ZERO : op1->GetRegNum(); - regNumber regOp2 = op2->isContained() ? REG_ZERO : op2->GetRegNum(); - int regs = (int)regOp1 | (((int)regOp2) << 5); + assert(!op2->isContained() || op2->IsIntegralConst(0)); + regNumber reg1 = op1->isContained() ? REG_ZERO : op1->GetRegNum(); + regNumber reg2 = op2->isContained() ? REG_ZERO : op2->GetRegNum(); + assert(emitter::isGeneralRegisterOrR0(reg1) && emitter::isGeneralRegisterOrR0(reg2)); + int regs = (int)reg1 | (((int)reg2) << 5); GetEmitter()->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); // If we cannot fall into the false target, emit a jump to it From 2cfd860461e147020b272170552aa63e82e9afb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 1 Aug 2025 12:13:10 +0200 Subject: [PATCH 21/36] Fix jtrue assert --- src/coreclr/jit/lsra.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 270aa90f96385c..339ad1fde1a6b6 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -9040,9 +9040,7 @@ void LinearScan::handleOutgoingCriticalEdges(BasicBlock* block) if (lastNode->OperIs(GT_JTRUE, GT_JCMP, GT_JTEST)) { - assert(lastNode->OperIs(GT_JTRUE) - ? !lastNode->gtGetOp1()->isContained() - : (!lastNode->gtGetOp1()->isContained() || !lastNode->gtGetOp2()->isContained())); + assert(!lastNode->OperIs(GT_JTRUE) || !lastNode->gtGetOp1()->isContained()); if (!lastNode->gtGetOp1()->isContained()) { GenTree* op = lastNode->gtGetOp1(); From fd9142eb5792f3259882314a01dae4ff312fe071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Mon, 4 Aug 2025 16:35:15 +0200 Subject: [PATCH 22/36] Lower the comparison again after reversal in OptimizeConstCompare --- src/coreclr/jit/lower.cpp | 4 ++++ src/coreclr/jit/lowerriscv64.cpp | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 1dac56c0b6be7b..a7aea601fae9f4 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4365,6 +4365,10 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) { GenTree* reversed = comp->gtReverseCond(op1); assert(reversed == op1); +#ifdef TARGET_RISCV64 + LowerNode(op1); + op1 = cmp->gtGetOp1(); +#endif } // Relops and SETCC can be either TYP_INT or TYP_LONG typed, so we diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index e68c5a97678bca..7be3fa10744071 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -211,6 +211,14 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) GenTree*& right = cmp->AsOp()->gtOp2; assert(cmp->OperIsCmpCompare() && varTypeUsesIntReg(left)); bool isReversed = false; + + if (left->IsIntegralConst() && !right->IsIntegralConst()) + { + // Normally const operand is ordered on the right. If we're here, it means we're lowering the second time + cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); + std::swap(left, right); + } + if (cmp->OperIs(GT_EQ, GT_NE)) { if (!right->IsIntegralConst(0)) From 95b38b763d25844bc1d29224ef26c29e7ce10982 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Tue, 5 Aug 2025 08:59:57 +0200 Subject: [PATCH 23/36] fix windows build --- src/coreclr/jit/emitriscv64.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/emitriscv64.cpp b/src/coreclr/jit/emitriscv64.cpp index 71bb7b5f3e41d1..37163a9f2c9f1b 100644 --- a/src/coreclr/jit/emitriscv64.cpp +++ b/src/coreclr/jit/emitriscv64.cpp @@ -1894,7 +1894,7 @@ unsigned emitter::emitOutputCall(const insGroup* ig, BYTE* dst, instrDesc* id, c // jalr t2 ssize_t imm = (ssize_t)(id->idAddr()->iiaAddr); - assert((uint64_t)(imm >> 32) <= 0x7fff); // RISC-V Linux Kernel SV48 + assert((uint64_t)(imm >> 32) <= 0x7fffUL); // RISC-V Linux Kernel SV48 int reg2 = (int)(imm & 1); imm -= reg2; From a08f58031dc2ffe85ed340941500a78b0d7fc31d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Tue, 5 Aug 2025 11:03:47 +0200 Subject: [PATCH 24/36] fix windows build --- src/coreclr/jit/emitriscv64.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/emitriscv64.cpp b/src/coreclr/jit/emitriscv64.cpp index 40e5177480cc43..9b123e1a794ae5 100644 --- a/src/coreclr/jit/emitriscv64.cpp +++ b/src/coreclr/jit/emitriscv64.cpp @@ -1895,7 +1895,7 @@ unsigned emitter::emitOutputCall(const insGroup* ig, BYTE* dst, instrDesc* id) } } - return dst - origDst; + return (unsigned)(dst - origDst); } void emitter::emitJumpDistBind() From 1f088577bbfd5cdd62a75fa4e5051dd04c49837d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Mon, 29 Sep 2025 13:41:51 +0200 Subject: [PATCH 25/36] Revert to doing FP comparison swaps and reversals in codegen --- src/coreclr/jit/codegenriscv64.cpp | 17 +++++++++++-- src/coreclr/jit/lower.cpp | 22 +++++++++-------- src/coreclr/jit/lower.h | 1 - src/coreclr/jit/lowerriscv64.cpp | 38 +++++------------------------- src/coreclr/jit/lsrariscv64.cpp | 3 +++ 5 files changed, 36 insertions(+), 45 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 53c0dcda21799a..a776cae381b685 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3135,10 +3135,21 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) { assert(!op1->isContainedIntOrIImmed() && !op2->isContainedIntOrIImmed()); assert(op1->TypeIs(op2->TypeGet())); - noway_assert((tree->gtFlags & GTF_RELOP_NAN_UN) == 0); + genTreeOps oper = tree->OperGet(); + + bool isUnordered = (tree->gtFlags & GTF_RELOP_NAN_UN) != 0; + if (isUnordered) + { + oper = GenTree::ReverseRelop(oper); + } + if (oper == GT_GT || oper == GT_GE) + { + oper = GenTree::SwapRelop(oper); + std::swap(op1, op2); + } instruction instr = INS_none; - switch (tree->OperGet()) + switch (oper) { case GT_LT: instr = (cmpSize == EA_4BYTE) ? INS_flt_s : INS_flt_d; @@ -3153,6 +3164,8 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) unreached(); } emit->emitIns_R_R_R(instr, cmpSize, targetReg, op1->GetRegNum(), op2->GetRegNum()); + if (isUnordered) + emit->emitIns_R_R_I(INS_xori, EA_8BYTE, targetReg, targetReg, 1); } else { diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index f8ed621ede8d02..34304ff0773e33 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4458,18 +4458,20 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) if (!BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE)) return cmp->gtNext; - bool isReversed = - varTypeIsFloating(cmp->gtGetOp1()) ? LowerAndReverseFloatingCompare(cmp) : LowerAndReverseIntegerCompare(cmp); - if (isReversed) + if (!varTypeIsFloating(cmp->gtGetOp1())) { - GenTree* one = comp->gtNewIconNode(1); - GenTree* notOp = comp->gtNewOperNode(GT_XOR, cmp->gtType, cmp, one); - BlockRange().InsertAfter(cmp, one, notOp); - one->SetContained(); - cmpUse.ReplaceWith(notOp); + if (LowerAndReverseIntegerCompare(cmp)) + { + GenTree* one = comp->gtNewIconNode(1); + GenTree* notOp = comp->gtNewOperNode(GT_XOR, cmp->gtType, cmp, one); + BlockRange().InsertAfter(cmp, one, notOp); + one->SetContained(); + cmpUse.ReplaceWith(notOp); + } + + if (!cmp->OperIsCmpCompare()) // comparison was optimized out to some other node + return cmp->gtNext; } - if (!cmp->OperIsCmpCompare()) // comparison was optimized out to some other node - return cmp->gtNext; #endif // TARGET_RISCV64 ContainCheckCompare(cmp->AsOp()); diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 3ee71f05feb365..0739b67ad6bcde 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -160,7 +160,6 @@ class Lowering final : public Phase GenTree* LowerCompare(GenTree* cmp); GenTree* LowerJTrue(GenTreeOp* jtrue); #ifdef TARGET_RISCV64 - bool LowerAndReverseFloatingCompare(GenTree* cmp); bool LowerAndReverseIntegerCompare(GenTree* cmp); void SignExtendIfNecessary(GenTree** arg); #endif diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 7be3fa10744071..b6bcce80ad8535 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -146,10 +146,13 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) { // branch if (cond) ---> branch if (cond != 0) GenCondition::Code code = GenCondition::NE; - if (cmp->OperIsCompare() && varTypeIsFloating(cmp->gtGetOp1())) + if (cmp->OperIsCompare() && varTypeIsFloating(cmp->gtGetOp1()) && (cmp->gtFlags & GTF_RELOP_NAN_UN) != 0) { - if (LowerAndReverseFloatingCompare(cmp)) - code = GenCondition::EQ; + // 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. + cmp->ChangeOper(GenTree::ReverseRelop(cmp->OperGet())); + cmp->gtFlags &= ~GTF_RELOP_NAN_UN; + code = GenCondition::EQ; } jcmp->gtCondition = GenCondition(code); jcmp->gtOp1 = cmp; @@ -166,35 +169,6 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) return jcmp->gtNext; } -//------------------------------------------------------------------------ -// LowerAndReverseFloatingCompare: lowers and reverses a floating-point comparison so that it matches the -// available instructions: "<", "<=", "==" (ordered only, register only) -// -// Arguments: -// cmp - the floating-point comparison to lower -// -// Returns: -// Whether the comparison was reversed. -// -bool Lowering::LowerAndReverseFloatingCompare(GenTree* cmp) -{ - assert(cmp->OperIsCmpCompare() && varTypeIsFloating(cmp->gtGetOp1())); - bool isUnordered = (cmp->gtFlags & GTF_RELOP_NAN_UN) != 0; - if (isUnordered) - { - // a CMP b (unordered) ---> !(a reversedCMP b (ordered)) - cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); - cmp->gtFlags &= ~GTF_RELOP_NAN_UN; - } - if (cmp->OperIs(GT_GT, GT_GE)) - { - cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); - std::swap(cmp->AsOp()->gtOp1, cmp->AsOp()->gtOp2); - } - assert(cmp->OperIs(GT_EQ, GT_LT, GT_LE)); - return isUnordered; -} - //------------------------------------------------------------------------ // LowerAndReverseIntegerCompare: lowers and reverses an integer comparison so that it matches the available // instructions: only "<" (with unsigned and immediate variants) diff --git a/src/coreclr/jit/lsrariscv64.cpp b/src/coreclr/jit/lsrariscv64.cpp index 8071a2a22cee9e..f001e03f84d37f 100644 --- a/src/coreclr/jit/lsrariscv64.cpp +++ b/src/coreclr/jit/lsrariscv64.cpp @@ -429,8 +429,11 @@ int LinearScan::BuildNode(GenTree* tree) break; case GT_EQ: + case GT_NE: case GT_LT: case GT_LE: + case GT_GE: + case GT_GT: case GT_JCMP: srcCount = BuildCmp(tree); break; From 3a411e5736c14920ff1c78af036567fedbbf4790 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Mon, 29 Sep 2025 15:58:49 +0200 Subject: [PATCH 26/36] Revert back to doing swapping and reversing for integer comparisons in codegen --- src/coreclr/jit/codegenriscv64.cpp | 28 +++++++++++++++++----- src/coreclr/jit/lower.cpp | 10 +------- src/coreclr/jit/lower.h | 2 +- src/coreclr/jit/lowerriscv64.cpp | 38 ++++++------------------------ 4 files changed, 31 insertions(+), 47 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index a776cae381b685..9131f2e3ccc464 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3108,7 +3108,7 @@ void CodeGen::genCkfinite(GenTree* treeNode) } //------------------------------------------------------------------------ -// genCodeForCompare: Produce code for a GT_EQ/GT_LT/GT_LE node. +// genCodeForCompare: Produce code for a GT_EQ/GT_NE/GT_LT/GT_LE/GT_GE/GT_GT node. // // Arguments: // tree - the node @@ -3131,14 +3131,15 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) assert(targetReg != REG_NA); assert(!tree->TypeIs(TYP_VOID)); + bool isReversed = false; if (varTypeIsFloating(op1Type)) { assert(!op1->isContainedIntOrIImmed() && !op2->isContainedIntOrIImmed()); assert(op1->TypeIs(op2->TypeGet())); genTreeOps oper = tree->OperGet(); - bool isUnordered = (tree->gtFlags & GTF_RELOP_NAN_UN) != 0; - if (isUnordered) + isReversed = (tree->gtFlags & GTF_RELOP_NAN_UN) != 0; + if (isReversed) { oper = GenTree::ReverseRelop(oper); } @@ -3164,12 +3165,24 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) unreached(); } emit->emitIns_R_R_R(instr, cmpSize, targetReg, op1->GetRegNum(), op2->GetRegNum()); - if (isUnordered) - emit->emitIns_R_R_I(INS_xori, EA_8BYTE, targetReg, targetReg, 1); } else { - noway_assert(tree->OperIs(GT_LT)); + noway_assert(tree->OperIs(GT_LT, GT_GT, GT_LE, GT_GE)); + genTreeOps oper = tree->OperGet(); + + isReversed = (oper == GT_LE || oper == GT_GE); + if (isReversed) + { + oper = GenTree::ReverseRelop(oper); + } + if (oper == GT_GT) + { + INDEBUG(oper = GenTree::SwapRelop(oper)); + std::swap(op1, op2); + } + assert(oper == GT_LT); // RISC-V only has set-less-than instructions + assert(!op1->isContainedIntOrIImmed() || op1->IsIntegralConst(0)); regNumber reg1 = op1->isContainedIntOrIImmed() ? REG_ZERO : op1->GetRegNum(); if (op2->isContainedIntOrIImmed()) @@ -3184,6 +3197,9 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) } } + if (isReversed) + emit->emitIns_R_R_I(INS_xori, EA_8BYTE, targetReg, targetReg, 1); + genProduceReg(tree); } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 34304ff0773e33..e96f851507a1f0 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4460,15 +4460,7 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) if (!varTypeIsFloating(cmp->gtGetOp1())) { - if (LowerAndReverseIntegerCompare(cmp)) - { - GenTree* one = comp->gtNewIconNode(1); - GenTree* notOp = comp->gtNewOperNode(GT_XOR, cmp->gtType, cmp, one); - BlockRange().InsertAfter(cmp, one, notOp); - one->SetContained(); - cmpUse.ReplaceWith(notOp); - } - + LowerIntegerCompare(cmp); if (!cmp->OperIsCmpCompare()) // comparison was optimized out to some other node return cmp->gtNext; } diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 0739b67ad6bcde..82dc5b4316339c 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -160,7 +160,7 @@ class Lowering final : public Phase GenTree* LowerCompare(GenTree* cmp); GenTree* LowerJTrue(GenTreeOp* jtrue); #ifdef TARGET_RISCV64 - bool LowerAndReverseIntegerCompare(GenTree* cmp); + void LowerIntegerCompare(GenTree* cmp); void SignExtendIfNecessary(GenTree** arg); #endif GenTree* LowerSelect(GenTreeConditional* cond); diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index b6bcce80ad8535..9155fffbb24d1e 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -71,6 +71,7 @@ bool Lowering::IsContainableImmed(GenTree* parentNode, GenTree* childNode) const case GT_XOR: return emitter::isValidSimm12(immVal); + case GT_GT: case GT_JCMP: case GT_CMPXCHG: case GT_XORR: @@ -170,28 +171,16 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) } //------------------------------------------------------------------------ -// LowerAndReverseIntegerCompare: lowers and reverses an integer comparison so that it matches the available -// instructions: only "<" (with unsigned and immediate variants) +// LowerIntegerCompare: lowers an integer comparison so that it matches the available instructions better // // Arguments: // cmp - the integer comparison to lower // -// Returns: -// Whether the comparison was reversed. -// -bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) +void Lowering::LowerIntegerCompare(GenTree* cmp) { GenTree*& left = cmp->AsOp()->gtOp1; GenTree*& right = cmp->AsOp()->gtOp2; assert(cmp->OperIsCmpCompare() && varTypeUsesIntReg(left)); - bool isReversed = false; - - if (left->IsIntegralConst() && !right->IsIntegralConst()) - { - // Normally const operand is ordered on the right. If we're here, it means we're lowering the second time - cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); - std::swap(left, right); - } if (cmp->OperIs(GT_EQ, GT_NE)) { @@ -253,39 +242,26 @@ bool Lowering::LowerAndReverseIntegerCompare(GenTree* cmp) BlockRange().Remove(left); BlockRange().Remove(right); cmp->BashToConst(1); - return false; + return; } right->AsIntConCommon()->SetIntegralValue(cmp->OperIs(GT_LE) ? value + 1 : value - 1); cmp->SetOperRaw(cmp->OperIs(GT_LE) ? GT_LT : GT_GT); } - else - { - // a <= b ---> !(a > b) - isReversed = true; - cmp->SetOperRaw(GenTree::ReverseRelop(cmp->OperGet())); - } - } - - if (cmp->OperIs(GT_GT)) - { - cmp->SetOperRaw(GenTree::SwapRelop(cmp->OperGet())); - std::swap(left, right); } - assert(cmp->OperIs(GT_LT)); - if (right->IsIntegralConst(0) && !right->AsIntCon()->ImmedValNeedsReloc(comp) && !cmp->IsUnsigned()) + if (cmp->OperIs(GT_LT) && right->IsIntegralConst(0) && !right->AsIntCon()->ImmedValNeedsReloc(comp) && + !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 isReversed; + return; } SignExtendIfNecessary(&left); SignExtendIfNecessary(&right); - return isReversed; } //------------------------------------------------------------------------ From f9228bd01316661e9fce847b8ce112f450c0574a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Tue, 30 Sep 2025 09:10:34 +0200 Subject: [PATCH 27/36] Remove double lowering --- src/coreclr/jit/lower.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index e96f851507a1f0..c6172618b4e9c1 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4366,10 +4366,6 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) { GenTree* reversed = comp->gtReverseCond(op1); assert(reversed == op1); -#ifdef TARGET_RISCV64 - LowerNode(op1); - op1 = cmp->gtGetOp1(); -#endif } // Relops and SETCC can be either TYP_INT or TYP_LONG typed, so we From 624e885950956811955366e710ad61cb646858ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Tue, 30 Sep 2025 13:13:20 +0200 Subject: [PATCH 28/36] Revert swapping back to codegen for JCMP --- src/coreclr/jit/codegenriscv64.cpp | 27 ++++++++++++++++++--------- src/coreclr/jit/lowerriscv64.cpp | 14 +++----------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 9131f2e3ccc464..f58ddc7f069c9a 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3223,9 +3223,26 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) assert(tree->TypeIs(TYP_VOID)); assert(tree->GetRegNum() == REG_NA); + GenTree* op1 = tree->gtGetOp1(); + GenTree* op2 = tree->gtGetOp2(); + assert(!op1->isUsedFromMemory()); + assert(!op2->isUsedFromMemory()); + assert(!op1->isContainedIntOrIImmed()); + assert(!op2->isContainedIntOrIImmed() || op2->IsIntegralConst(0)); + + GenCondition cond = tree->gtCondition; genConsumeOperands(tree); + regNumber reg1 = op1->GetRegNum(); + regNumber reg2 = op2->isContainedIntOrIImmed() ? REG_ZERO : op2->GetRegNum(); + if (cond.Is(GenCondition::SGT, GenCondition::UGT, GenCondition::SLE, GenCondition::ULE)) + { + // ">" and "<=" are achieved by swapping inputs for "<" and ">=" + cond = GenCondition::Swap(cond); + std::swap(reg1, reg2); + } + instruction ins = INS_invalid; - switch (tree->gtCondition.GetCode()) + switch (cond.GetCode()) { case GenCondition::EQ: ins = INS_beq; @@ -3250,14 +3267,6 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) break; } - GenTree* op1 = tree->gtGetOp1(); - GenTree* op2 = tree->gtGetOp2(); - assert(!op1->isUsedFromMemory()); - assert(!op2->isUsedFromMemory()); - assert(!op1->isContained() || op1->IsIntegralConst(0)); - assert(!op2->isContained() || op2->IsIntegralConst(0)); - regNumber reg1 = op1->isContained() ? REG_ZERO : op1->GetRegNum(); - regNumber reg2 = op2->isContained() ? REG_ZERO : op2->GetRegNum(); assert(emitter::isGeneralRegisterOrR0(reg1) && emitter::isGeneralRegisterOrR0(reg2)); int regs = (int)reg1 | (((int)reg2) << 5); GetEmitter()->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 9155fffbb24d1e..95f29bc39f143f 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -135,12 +135,6 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) jcmp->gtCondition = GenCondition::FromIntegralRelop(cmp); jcmp->gtOp1 = cmp->gtGetOp1(); jcmp->gtOp2 = cmp->gtGetOp2(); - if (cmp->OperIs(GT_GT, GT_LE)) - { - // ">" and "<=" are achieved by swapping inputs for "<" and ">=" - jcmp->gtCondition = GenCondition::Swap(jcmp->gtCondition); - std::swap(jcmp->gtOp1, jcmp->gtOp2); - } BlockRange().Remove(cmp); } else @@ -161,11 +155,9 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) BlockRange().InsertBefore(jcmp, jcmp->gtOp2); } - for (GenTree** op : {&jcmp->gtOp1, &jcmp->gtOp2}) - { - SignExtendIfNecessary(op); - CheckImmedAndMakeContained(jcmp, *op); - } + SignExtendIfNecessary(&jcmp->gtOp1); + SignExtendIfNecessary(&jcmp->gtOp2); + CheckImmedAndMakeContained(jcmp, jcmp->gtOp2); return jcmp->gtNext; } From 5f13399ad81376c54f249d4b1627bf366b4032e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Tue, 30 Sep 2025 13:41:26 +0200 Subject: [PATCH 29/36] Add GE and LE to immediates --- src/coreclr/jit/lowerriscv64.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 95f29bc39f143f..7f433a3d441468 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -65,6 +65,7 @@ bool Lowering::IsContainableImmed(GenTree* parentNode, GenTree* childNode) const switch (parentNode->OperGet()) { case GT_LT: + case GT_GE: case GT_ADD: case GT_AND: case GT_OR: @@ -72,6 +73,7 @@ bool Lowering::IsContainableImmed(GenTree* parentNode, GenTree* childNode) const return emitter::isValidSimm12(immVal); case GT_GT: + case GT_LE: case GT_JCMP: case GT_CMPXCHG: case GT_XORR: From 6ed4268af7e34672ddb8c60ee7bd2ab7b77e0a1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Tue, 30 Sep 2025 15:37:07 +0200 Subject: [PATCH 30/36] Move EQ|NE(a, 0) back to codegen --- src/coreclr/jit/codegenriscv64.cpp | 14 +++++++++++--- src/coreclr/jit/lowerriscv64.cpp | 5 ++--- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index f58ddc7f069c9a..5c7ed560dda05f 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3115,6 +3115,7 @@ void CodeGen::genCkfinite(GenTree* treeNode) // void CodeGen::genCodeForCompare(GenTreeOp* tree) { + assert(tree->OperIsCmpCompare()); GenTree* op1 = tree->gtOp1; GenTree* op2 = tree->gtOp2; var_types op1Type = genActualType(op1->TypeGet()); @@ -3168,8 +3169,15 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) } else { - noway_assert(tree->OperIs(GT_LT, GT_GT, GT_LE, GT_GE)); + bool isUnsigned = tree->IsUnsigned(); + genTreeOps oper = tree->OperGet(); + if (oper == GT_EQ || oper == GT_NE) + { + assert(op2->isContainedIntOrIImmed() && op2->IsIntegralConst(0)); + oper = (oper == GT_EQ) ? GT_LE : GT_GT; + isUnsigned = true; + } isReversed = (oper == GT_LE || oper == GT_GE); if (isReversed) @@ -3187,12 +3195,12 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) regNumber reg1 = op1->isContainedIntOrIImmed() ? REG_ZERO : op1->GetRegNum(); if (op2->isContainedIntOrIImmed()) { - instruction slti = tree->IsUnsigned() ? INS_sltiu : INS_slti; + instruction slti = isUnsigned ? INS_sltiu : INS_slti; emit->emitIns_R_R_I(slti, EA_PTRSIZE, targetReg, reg1, op2->AsIntCon()->gtIconVal); } else { - instruction slt = tree->IsUnsigned() ? INS_sltu : INS_slt; + instruction slt = isUnsigned ? INS_sltu : INS_slt; emit->emitIns_R_R_R(slt, EA_PTRSIZE, targetReg, reg1, op2->GetRegNum()); } } diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 7f433a3d441468..effaa17a2b72d9 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -72,6 +72,8 @@ bool Lowering::IsContainableImmed(GenTree* parentNode, GenTree* childNode) const case GT_XOR: return emitter::isValidSimm12(immVal); + case GT_EQ: + case GT_NE: case GT_GT: case GT_LE: case GT_JCMP: @@ -206,9 +208,6 @@ void Lowering::LowerIntegerCompare(GenTree* cmp) BlockRange().InsertBefore(cmp, left, right); ContainCheckBinary(left->AsOp()); } - // a != 0 ---> a > 0 (unsigned) - cmp->SetOperRaw(cmp->OperIs(GT_EQ) ? GT_LE : GT_GT); - cmp->SetUnsigned(); } if (cmp->OperIs(GT_LE, GT_GE)) From 97f90dd4d9a729f2392223b97887567973df44ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Wed, 1 Oct 2025 14:42:03 +0200 Subject: [PATCH 31/36] Handle a <= 0 ---> a < 1 in codegen --- src/coreclr/jit/codegenriscv64.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 5c7ed560dda05f..63040c4afd5682 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3179,6 +3179,13 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree) isUnsigned = true; } + if (oper == GT_LE && op2->isContainedIntOrIImmed()) + { + oper = GT_LT; + assert(op2->AsIntCon()->gtIconVal == 0); + op2->AsIntCon()->gtIconVal = 1; + } + isReversed = (oper == GT_LE || oper == GT_GE); if (isReversed) { From cb6d150fa5c2c0c071ca5c5f1a82b41dc9d59b79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 2 Oct 2025 09:29:08 +0200 Subject: [PATCH 32/36] Remove general optimizations --- src/coreclr/jit/lower.cpp | 16 +++--- src/coreclr/jit/lower.h | 4 +- src/coreclr/jit/lowerriscv64.cpp | 97 +++++++++++++------------------- 3 files changed, 47 insertions(+), 70 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index bb4c9145667a2c..24a136a83d9fa6 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4512,16 +4512,14 @@ GenTree* Lowering::LowerCompare(GenTree* cmp) } } #elif defined(TARGET_RISCV64) - // Branches will be lowered in LowerJTrue - LIR::Use cmpUse; - if (!BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE)) - return cmp->gtNext; - - if (!varTypeIsFloating(cmp->gtGetOp1())) + if (varTypeUsesIntReg(cmp->gtGetOp1())) { - LowerIntegerCompare(cmp); - if (!cmp->OperIsCmpCompare()) // comparison was optimized out to some other node - return cmp->gtNext; + 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 diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index c72e2d0007a142..09687301a33679 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -160,8 +160,8 @@ class Lowering final : public Phase GenTree* LowerCompare(GenTree* cmp); GenTree* LowerJTrue(GenTreeOp* jtrue); #ifdef TARGET_RISCV64 - void LowerIntegerCompare(GenTree* cmp); - void SignExtendIfNecessary(GenTree** arg); + GenTree* LowerSavedIntegerCompare(GenTree* cmp); + void SignExtendIfNecessary(GenTree** arg); #endif GenTree* LowerSelect(GenTreeConditional* cond); bool TryLowerConditionToFlagsNode(GenTree* parent, diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index effaa17a2b72d9..2857564c906200 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -159,62 +159,51 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) BlockRange().InsertBefore(jcmp, jcmp->gtOp2); } - SignExtendIfNecessary(&jcmp->gtOp1); - SignExtendIfNecessary(&jcmp->gtOp2); - CheckImmedAndMakeContained(jcmp, jcmp->gtOp2); + // Comparisons fused with branches don't have immediates, re-evaluate containment for 'zero' register + jcmp->gtOp2->ClearContained(); + ContainCheckCompare(jcmp); return jcmp->gtNext; } //------------------------------------------------------------------------ -// LowerIntegerCompare: lowers an integer comparison so that it matches the available instructions better +// LowerSavedIntegerCompare: lowers a integer comparison saved to a register so that it matches the available +// instructions better // // Arguments: // cmp - the integer comparison to lower // -void Lowering::LowerIntegerCompare(GenTree* cmp) +// 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 + if (LIR::Use cmpUse; !BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE)) + return cmp; + GenTree*& left = cmp->AsOp()->gtOp1; GenTree*& right = cmp->AsOp()->gtOp2; assert(cmp->OperIsCmpCompare() && varTypeUsesIntReg(left)); - if (cmp->OperIs(GT_EQ, GT_NE)) + if (cmp->OperIs(GT_EQ, GT_NE) && !right->IsIntegralConst(0)) { - if (!right->IsIntegralConst(0)) - { - // 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 (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()); - } + // Only equality with zero is supported + // a == b ---> (a - b) == 0 + var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_I_IMPL; + left = comp->gtNewOperNode(GT_SUB, type, left, right); + right = comp->gtNewZeroConNode(type); + BlockRange().InsertBefore(cmp, left, right); + LowerNode(left); } - if (cmp->OperIs(GT_LE, GT_GE)) + if (right->IsIntegralConst() && !right->AsIntConCommon()->ImmedValNeedsReloc(comp)) { - if (right->IsIntegralConst() && !right->AsIntCon()->ImmedValNeedsReloc(comp)) + if (cmp->OperIs(GT_LE, GT_GE)) { // a <= C ---> a < C+1 + // a >= C ---> a > C-1 INT64 value = right->AsIntConCommon()->IntegralValue(); bool isOverflow; @@ -230,31 +219,24 @@ void Lowering::LowerIntegerCompare(GenTree* cmp) ? CheckedOps::SubOverflows((INT32)value, (INT32)1, cmp->IsUnsigned()) : CheckedOps::SubOverflows((INT64)value, (INT64)1, cmp->IsUnsigned()); } - if (isOverflow) + if (!isOverflow) { - BlockRange().Remove(left); - BlockRange().Remove(right); - cmp->BashToConst(1); - return; + right->AsIntConCommon()->SetIntegralValue(cmp->OperIs(GT_LE) ? value + 1 : value - 1); + cmp->SetOperRaw(cmp->OperIs(GT_LE) ? GT_LT : GT_GT); } - 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) && !right->AsIntCon()->ImmedValNeedsReloc(comp) && - !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; + 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; + } } - - SignExtendIfNecessary(&left); - SignExtendIfNecessary(&right); + return cmp; } //------------------------------------------------------------------------ @@ -1234,9 +1216,6 @@ void Lowering::ContainCheckCast(GenTreeCast* node) // void Lowering::ContainCheckCompare(GenTreeOp* cmp) { - if (cmp->gtOp1->IsIntegralConst(0) && !cmp->gtOp1->AsIntCon()->ImmedValNeedsReloc(comp)) - MakeSrcContained(cmp, cmp->gtOp1); - CheckImmedAndMakeContained(cmp, cmp->gtOp2); } From fd28d1598ac202650a8fa9818d1120be645836ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Thu, 2 Oct 2025 15:26:36 +0200 Subject: [PATCH 33/36] Bring back zero register op1 in branches --- src/coreclr/jit/codegenriscv64.cpp | 5 ++--- src/coreclr/jit/lowerriscv64.cpp | 7 +++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index af6e1919d2449e..a3febcdd804c7d 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -3252,16 +3252,15 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree) GenTree* op2 = tree->gtGetOp2(); assert(!op1->isUsedFromMemory()); assert(!op2->isUsedFromMemory()); - assert(!op1->isContainedIntOrIImmed()); + assert(!op1->isContainedIntOrIImmed() || op1->IsIntegralConst(0)); assert(!op2->isContainedIntOrIImmed() || op2->IsIntegralConst(0)); GenCondition cond = tree->gtCondition; genConsumeOperands(tree); - regNumber reg1 = op1->GetRegNum(); + regNumber reg1 = op1->isContainedIntOrIImmed() ? REG_ZERO : op1->GetRegNum(); regNumber reg2 = op2->isContainedIntOrIImmed() ? REG_ZERO : op2->GetRegNum(); if (cond.Is(GenCondition::SGT, GenCondition::UGT, GenCondition::SLE, GenCondition::ULE)) { - // ">" and "<=" are achieved by swapping inputs for "<" and ">=" cond = GenCondition::Swap(cond); std::swap(reg1, reg2); } diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 2857564c906200..dc05e76e8da296 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -160,8 +160,8 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) } // Comparisons fused with branches don't have immediates, re-evaluate containment for 'zero' register - jcmp->gtOp2->ClearContained(); - ContainCheckCompare(jcmp); + if (!CheckImmedAndMakeContained(jcmp, jcmp->gtOp2)) + jcmp->gtOp2->ClearContained(); return jcmp->gtNext; } @@ -1216,6 +1216,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); } From 78340584ebb76741fc0b69f361a11129f9495944 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 3 Oct 2025 09:53:01 +0200 Subject: [PATCH 34/36] Bring back a - C ---> a + (-C) --- src/coreclr/jit/lowerriscv64.cpp | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index dc05e76e8da296..3cb747641c18a1 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -191,14 +191,33 @@ GenTree* Lowering::LowerSavedIntegerCompare(GenTree* cmp) { // Only equality with zero is supported // a == b ---> (a - b) == 0 - var_types type = genActualTypeIsInt(left) ? TYP_INT : TYP_I_IMPL; - left = comp->gtNewOperNode(GT_SUB, type, left, right); - right = comp->gtNewZeroConNode(type); + 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); - LowerNode(left); + ContainCheckBinary(left->AsOp()); } - if (right->IsIntegralConst() && !right->AsIntConCommon()->ImmedValNeedsReloc(comp)) + if (!right->TypeIs(TYP_BYREF) && right->IsIntegralConst() && !right->AsIntConCommon()->ImmedValNeedsReloc(comp)) { if (cmp->OperIs(GT_LE, GT_GE)) { From da41847b1d47e9a5a221efcb3a3832f794f682f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Sowi=C5=84ski?= Date: Fri, 17 Oct 2025 12:24:52 +0200 Subject: [PATCH 35/36] Jit coding conventions Co-authored-by: Jakob Botsch Nielsen --- src/coreclr/jit/lowerriscv64.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 3cb747641c18a1..1a3d9d2a255f27 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -180,7 +180,8 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) GenTree* Lowering::LowerSavedIntegerCompare(GenTree* cmp) { // Branches have a full range of comparisons, these transformations would be counter-productive - if (LIR::Use cmpUse; !BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE)) + LIR::Use cmpUse; + if (!BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE)) return cmp; GenTree*& left = cmp->AsOp()->gtOp1; From 4a96957880896cd70bcdd4bb23d1e73c4216c700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Sowi=C5=84ski?= Date: Fri, 17 Oct 2025 13:06:32 +0200 Subject: [PATCH 36/36] format --- src/coreclr/jit/lowerriscv64.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 1a3d9d2a255f27..afad339cc76a68 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -180,7 +180,7 @@ GenTree* Lowering::LowerJTrue(GenTreeOp* jtrue) GenTree* Lowering::LowerSavedIntegerCompare(GenTree* cmp) { // Branches have a full range of comparisons, these transformations would be counter-productive - LIR::Use cmpUse; + LIR::Use cmpUse; if (!BlockRange().TryGetUse(cmp, &cmpUse) || cmpUse.User()->OperIs(GT_JTRUE)) return cmp;