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

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2173
 
+  if (Add)
+    CurDAG->UpdateNodeOperands(Add.getNode(), Add.getOperand(AddBaseIdx),
----------------
craig.topper wrote:
> 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.
Thanks for your investigation. It looks like UpdateNodeOperands() will return the existing node and it does not update the operands as we expected here.

Does it mean the following other UpdateNodeOperands() has similar problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124231