[PATCH] D79690: [RISCV] Fold ADDIs into load/stores with nonzero offsets

[Sam Elliott via Phabricator via llvm-commits]

lenary added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:219-224
+      int64_t Offset1 = Const->getSExtValue();
+      int64_t CombinedOffset = Offset1 + Offset2;
+      if (!isInt<12>(CombinedOffset))
+        continue;
+      ImmOperand = CurDAG->getTargetConstant(CombinedOffset, SDLoc(ImmOperand),
+                                             ImmOperand.getValueType());
----------------
luismarques wrote:
> lenary wrote:
> > I don't understand where this code is being tested. Can you point to a testcase that changes because of this change?
> That's the situation I mention in the review summary. That case doesn't seem to currently be generated by LLVM, so I couldn't generate a test that would be impacted by it. I can try to look harder or see if e.g. a MIR test could cover that case. Since the code was reasonably straightforward and the alternative (skipping on 0) was actually being harder to document in the code comments I kept the optimization, since it should be reasonably clear to reason about it being correct or not, despite the lack of specific test.
I though I had come up with two testcases for this, but I see I haven't, because LLVM never makes a selectiondag the equivalent of:

```
    t0: i32 = LUI <ConstAHi>
  t1: i32 = ADDI t0, <ConstALo>
t2: i32 = LW t1, 0
t3: i32 = LW t1, 4
```

instead it creates:
```
    t0: i32 = LUI <ConstAHi>
  t1: i32 = ADDI t0, <ConstALo>
t2: i32 = LW t1, 0
    t3: i32 = LUI <ConstBHi>
  t4: i32 = ADDI <ConstBLo>
t5: i32 = LW t4, 0
```

This means you still need to do this fold, but the case where the offset to the `LW` not being zero is not exercised by LLVM.

In any case, if you want it, here are two testcases, which. show you cannot skip the fold in the ConstantSDNode case, but also that your additions to the ConstantSDNode are not tested:
```
define i64 @load_const_ok() nounwind {
entry:
  %0 = load i64, i64* inttoptr (i32 2040 to i64*)
  ret i64 %0
}

define i64 @load_cost_overflow() nounwind {
entry:
  %0 = load i64, i64* inttoptr (i64 2044 to i64*)
  ret i64 %0
}
```

I'm not sure we test this kind of folding anywhere else, so I think we should add this to the testcases you add for this PR, even though there are no changes to the generated assembly after your patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79690/new/

https://reviews.llvm.org/D79690