[RISCV] Fix ICE in isDesirableToCommuteWithShift

[lenary]

Summary

There was an error being thrown from isDesirableToCommuteWithShift in
some tests. This was tracked down to the method being called before
legalisation, with an extended value type, not a machine value type.

In the case I diagnosed, the error was only hit with an instruction sequence
involving i24s in the add and shift. i24 is not a Machine ValueType, it is
instead an Extended ValueType which was causing the issue.

I have added a test to cover this case, and fixed the error in the callback.

[asb]

LGTM, thanks!

[apazos]

llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp

1010

Sam, I just noted when building compiler-rt (/compiler-rt/lib/builtins/addtf3.c) in debug mode, it crashes in this line.

The reason is ShiftedC1Int bitwidth is 128, but APInt getSExtValue expects a 64 bit quantity.

Can you take a look?

One solution is:

• a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp

+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -1003,6 +1003,8 @@ bool RISCVTargetLowering::isDesirableToCommuteWithShift(

if (C1 && C2) {
  APInt C1Int = C1->getAPIntValue();
  APInt ShiftedC1Int = C1Int << C2->getAPIntValue();

+ if (ShiftedC1Int.getBitWidth() > 64)
+ return false;

Reduced test case:

define void @test(i128* %a) {
entry:

%load = load i128, i128* %a, align 16
%or = or i128 %load, 5192296858534827628530496329220096
%shl = shl i128 %or, 3
store i128 %shl, i128* %a, align 16

ret void
}

[lenary]

llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp

1010

Thanks for letting me know. I have prepared a patch for this, including a variant of your testcase (which I have verified currently causes the same ICE you're seeing). It is D66081

llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp

 bool RISCVTargetLowering::isDesirableToCommuteWithShift(
     const SDNode *N, CombineLevel Level) const {
   // The following folds are only desirable if `(OP _, c1 << c2)` can be
   // materialised in fewer instructions than `(OP _, c1)`:
   //
   //   (shl (add x, c1), c2) -> (add (shl x, c2), c1 << c2)
   //   (shl (or x, c1), c2) -> (or (shl x, c2), c1 << c2)
   SDValue N0 = N->getOperand(0);
-  MVT Ty = N0.getSimpleValueType();
+  EVT Ty = N0.getValueType();
   if (Ty.isScalarInteger() &&
       (N0.getOpcode() == ISD::ADD || N0.getOpcode() == ISD::OR)) {
     auto *C1 = dyn_cast<ConstantSDNode>(N0->getOperand(1));
     auto *C2 = dyn_cast<ConstantSDNode>(N->getOperand(1));
     if (C1 && C2) {
       APInt C1Int = C1->getAPIntValue();
       APInt ShiftedC1Int = C1Int << C2->getAPIntValue();
 
       // We can materialise `c1 << c2` into an add immediate, so it's "free",
       // and the combine should happen, to potentially allow further combines
       // later.
       if (isLegalAddImmediate(ShiftedC1Int.getSExtValue()))
         return true;
 
       // We can materialise `c1` in an add immediate, so it's "free", and the
       // combine should be prevented.
       if (isLegalAddImmediate(C1Int.getSExtValue()))
         return false;
 
       // Neither constant will fit into an immediate, so find materialisation

llvm/trunk/test/CodeGen/RISCV/add-before-shl.ll

 ; RV64I-NEXT:    slli a0, a0, 48
 ; RV64I-NEXT:    srai a0, a0, 48
 ; RV64I-NEXT:    ret
   %1 = add i32 %a, 32767
   %2 = shl i32 %1, 16
   %3 = ashr i32 %2, 16
   ret i32 %3
 }
+
+define signext i24 @add_non_machine_type(i24 signext %a) nounwind {
+; RV32I-LABEL: add_non_machine_type:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    addi a0, a0, 256
+; RV32I-NEXT:    slli a0, a0, 20
+; RV32I-NEXT:    srai a0, a0, 8
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: add_non_machine_type:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    addi a0, a0, 256
+; RV64I-NEXT:    slli a0, a0, 52
+; RV64I-NEXT:    srai a0, a0, 40
+; RV64I-NEXT:    ret
+  %1 = add i24 %a, 256
+  %2 = shl i24 %1, 12
+  ret i24 %2
+}