[RISCV] Support LUI+ADDIW in doPeepholeLoadStoreADDI.

[craig.topper]

Summary

This fixes an inconsistency between RV32 and RV64. Still considering
trying to do this peephole during isel, but wanted to fix the
inconsistency first.

[craig.topper]

Upload whole patch.

[reames]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2248

Correct me if I'm wrong here, but isn't this the same as proving we could replace the ADDIW with an ADDI? If so, should that be done somewhere else generically?

llvm/test/CodeGen/RISCV/fold-addi-loadstore.ll

321

Can you precommit these please? I need to see the current code to wrap my head around the change,

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2248

Yes. So we definitely teach constant materialization to emit LUI+ADDI instead of LUI+ADDIW for more cases. And update a bunch of tests.

But I think we would then need to teach the RISCVSExtWRemoval pass that LUI+ADDI produces a sign extended result most of the time. It already knows ADDIW always produces a sign extended result. So as long as we always emit LUI+ADDIW we didn't need a special case.

So we'd just be moving the problem around.

Since I also wrote something similar to this patch for RISCVMergeBaseOffset D126729. Maybe that's another vote in favor of changing so we only have to deal with it in RISCVSExtWRemoval?

[reames]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2248

Let me ask this differently, why isn't RISCVSExtWRemoval using ComputeNumSignBits? Do we not have an equivalent of that for MI?

Quick digging shows that no we don't appear to have that. Unless, maybe I'm missing it?

Yeah, so I'm leaning towards canonicalizing towards ADDI, and making this RISCVSExtWRemoval.cpp's problem.

To be clear, I'm fine landing this patch as is (now that I actually am clear on what it's doing), and *then* working in that direction. Not urgent, not even required immediate followup.

Could you put a comment in this code which makes it very clear this is matching the case where ADDIW is the same as ADDI?

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2248

Let me ask this differently, why isn't RISCVSExtWRemoval using ComputeNumSignBits? Do we not have an equivalent of that for MI?

Quick digging shows that no we don't appear to have that. Unless, maybe I'm missing it?

We don't have that. And if we did we'd still have to add LUI+ADDI as a special case in it.

Yeah, so I'm leaning towards canonicalizing towards ADDI, and making this RISCVSExtWRemoval.cpp's problem.

To be clear, I'm fine landing this patch as is (now that I actually am clear on what it's doing), and *then* working in that direction. Not urgent, not even required immediate followup.

Could you put a comment in this code which makes it very clear this is matching the case where ADDIW is the same as ADDI?

Sure.

[craig.topper]

Improve comment. Rebase on pre-committed tests

[craig.topper]

Improve comment on the negative test.

[reames]

LGTM

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

bool RISCVDAGToDAGISel::doPeepholeLoadStoreADDI(SDNode *N) {
   if (!isa<ConstantSDNode>(N->getOperand(OffsetOpIdx)))
     return false;
 
   SDValue Base = N->getOperand(BaseOpIdx);
 
   if (!Base.isMachineOpcode())
     return false;
 
+  if (Base.getMachineOpcode() == RISCV::ADDI) {
     // If the base is an ADDI, we can merge it in to the load/store.
-  if (Base.getMachineOpcode() != RISCV::ADDI)
-    return false;
+  } else if (Base.getMachineOpcode() == RISCV::ADDIW &&
+             isa<ConstantSDNode>(Base.getOperand(1)) &&
+             Base.getOperand(0).isMachineOpcode() &&
+             Base.getOperand(0).getMachineOpcode() == RISCV::LUI &&
+             isa<ConstantSDNode>(Base.getOperand(0).getOperand(0))) {
+    // ADDIW can be merged if it's part of LUI+ADDIW constant materialization
+    // and LUI+ADDI would have produced the same result. This is true for all
+    // simm32 values except 0x7ffff800-0x7fffffff.
+    int64_t Offset =
+      SignExtend64<32>(Base.getOperand(0).getConstantOperandVal(0) << 12);
+    Offset += cast<ConstantSDNode>(Base.getOperand(1))->getSExtValue();
+    if (!isInt<32>(Offset))
+      return false;
+  } else
 
   SDValue ImmOperand = Base.getOperand(1);
   uint64_t Offset2 = N->getConstantOperandVal(OffsetOpIdx);
 
   if (auto *Const = dyn_cast<ConstantSDNode>(ImmOperand)) {
     int64_t Offset1 = Const->getSExtValue();
     int64_t CombinedOffset = Offset1 + Offset2;
     if (!isInt<12>(CombinedOffset))

llvm/test/CodeGen/RISCV/fold-addi-loadstore.ll

 ; RV64I:       # %bb.0: # %entry
 ; RV64I-NEXT:    ld a0, 2044(zero)
 ; RV64I-NEXT:    ret
 entry:
   %0 = load i64, i64* inttoptr (i64 2044 to i64*)
   ret i64 %0
 }
 
 define dso_local i32 @load_const_medium() nounwind {
 ; RV32I-LABEL: load_const_medium:
 ; RV32I:       # %bb.0: # %entry
 ; RV32I-NEXT:    lui a0, 1
 ; RV32I-NEXT:    lw a0, -16(a0)
 ; RV32I-NEXT:    ret
 ;
 ; RV64I-LABEL: load_const_medium:
 ; RV64I:       # %bb.0: # %entry
 ; RV64I-NEXT:    lui a0, 1
-; RV64I-NEXT:    addiw a0, a0, -16
-; RV64I-NEXT:    lw a0, 0(a0)
+; RV64I-NEXT:    lw a0, -16(a0)
 ; RV64I-NEXT:    ret
 entry:
   %0 = load i32, i32* inttoptr (i64 4080 to i32*)
   ret i32 %0
 }
 
+; The constant here is 0x7ffff800, this value requires LUI+ADDIW on RV64,
+; LUI+ADDI would produce a different constant so we can't fold into the load
+; offset.
 define dso_local i32 @load_const_large() nounwind {
 ; RV32I-LABEL: load_const_large:
 ; RV32I:       # %bb.0: # %entry
 ; RV32I-NEXT:    lui a0, 524288
 ; RV32I-NEXT:    lw a0, -2048(a0)
 ; RV32I-NEXT:    ret
 ;
 ; RV64I-LABEL: load_const_large: