[RISCV] When ISD::SETUGT && Imm == -1, processed before lowering

[reames]

Please follow LLVM development policies. Add a description of the change, and tests which demonstrate it.

[craig.topper]

In D132373#3739683, @reames wrote:

Please follow LLVM development policies. Add a description of the change, and tests which demonstrate it.

This is removing code that I added out of paranoia. I agree it should never happen.

At the very least it needs a comment and an assert.

[Miss_Grape]

In D132373#3739786, @craig.topper wrote:

In D132373#3739683, @reames wrote:

Please follow LLVM development policies. Add a description of the change, and tests which demonstrate it.

This is removing code that I added out of paranoia. I agree it should never happen.

At the very least it needs a comment and an assert.

I think this scenario has already been processed in the previous optimization stage. What do you think would be more appropriate to change?

[craig.topper]

This comment was removed by craig.topper.

[craig.topper]

In D132373#3745011, @Miss_Grape wrote:

In D132373#3739786, @craig.topper wrote:

In D132373#3739683, @reames wrote:

Please follow LLVM development policies. Add a description of the change, and tests which demonstrate it.

This is removing code that I added out of paranoia. I agree it should never happen.

At the very least it needs a comment and an assert.

I think this scenario has already been processed in the previous optimization stage. What do you think would be more appropriate to change?

I suggested we replace with

// X > -1 should have been replaced with false.
assert((CCVal != ISD::SETUGT || Imm != -1) && "Missing canonicalization");

[Miss_Grape]

In D132373#3746066, @craig.topper wrote:

In D132373#3745011, @Miss_Grape wrote:

In D132373#3739786, @craig.topper wrote:

In D132373#3739683, @reames wrote:

Please follow LLVM development policies. Add a description of the change, and tests which demonstrate it.

This is removing code that I added out of paranoia. I agree it should never happen.

At the very least it needs a comment and an assert.

I think this scenario has already been processed in the previous optimization stage. What do you think would be more appropriate to change?

I suggested we replace with

// X > -1 should have been replaced with false.
assert((CCVal != ISD::SETUGT || Imm != -1) && "Missing canonicalization");

Done

[craig.topper]

LGTM

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

if (OpVT.isScalarInteger()) {
 
       // If the RHS is a constant in the range [-2049, 0) or (0, 2046], we can
       // convert this to the equivalent of (set(u)ge X, C+1) by using
       // (xori (slti(u) X, C+1), 1). This avoids materializing a small constant
       // in a register.
       if (isa<ConstantSDNode>(RHS)) {
         int64_t Imm = cast<ConstantSDNode>(RHS)->getSExtValue();
         if (Imm != 0 && isInt<12>((uint64_t)Imm + 1)) {
-          // If this is an unsigned compare and the constant is -1, incrementing
-          // the constant would change behavior. The result should be false.
-          if (CCVal == ISD::SETUGT && Imm == -1)
-            return DAG.getConstant(0, DL, VT);
+          // X > -1 should have been replaced with false.
+          assert((CCVal != ISD::SETUGT || Imm != -1) &&
+                 "Missing canonicalization");
           // Using getSetCCSwappedOperands will convert SET(U)GT->SET(U)LT.
           CCVal = ISD::getSetCCSwappedOperands(CCVal);
           SDValue SetCC = DAG.getSetCC(
               DL, VT, LHS, DAG.getConstant(Imm + 1, DL, OpVT), CCVal);
           return DAG.getLogicalNOT(DL, SetCC, VT);
         }
       }