[PATCH] D124231: [RISCV] Merge addi into load/store as there is a ADD between them

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2173
 
+  if (Add)
+    CurDAG->UpdateNodeOperands(Add.getNode(), Add.getOperand(AddBaseIdx),
----------------
craig.topper wrote:
> I'm a little concerned about the use of UpdateNodeOperands in this function. If the update matches an existing node, it will delete the original node and return a new node. If that happens here, `Add` will no longer point to a valid node after the call.
> 
> I'm going to do some experiments.
As I feared, this test

```
define i32 @lw_far_local(i32* %a)  {                                             
  %1 = getelementptr inbounds i32, i32* %a, i64 4095                             
  %2 = load volatile i32, i32* %1                                                
  %3 = getelementptr inbounds i32, i32* %a, i64 4094                             
  %4 = load volatile i32, i32* %3                                                
  %5 = mul i32 %2, %4                                                            
  ret i32 %5                                                                     
}
```

miscompiles into this with -O0. I needed -O0 to get CodeGenPrepare to not rewrite the GEPs.

```
        mv      a1, a0
        lui     a2, 4
        addi    a0, a2, -4
        add     a0, a1, a0
        lw      a0, -4(a0) <- the offset for this is (4 << 12) + -4 + -4 + a1 making it the same offset for the next load. It should be (4 << 12) + -4 + a1
        add     a1, a1, a2
        lw      a1, -8(a1)
        mul     a0, a0, a1
        ret
```

This appears to be fixable by changing.

```
if (Add)
  CurDAG->UpdateNodeOperands(Add.getNode(), Add.getOperand(AddBaseIdx),
                             Base.getOperand(0));
```

to

```
if (Add)
  Add = SDValue(CurDAG->UpdateNodeOperands(Add.getNode(), Add.getOperand(AddBaseIdx),
                                           Base.getOperand(0)), 0);
```

But I'm not sure that's the only way this can break.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124231