[PATCH] D126986: [RISCV] Support LUI+ADDIW in doPeepholeLoadStoreADDI.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2248
+    Offset += cast<ConstantSDNode>(Base.getOperand(1))->getSExtValue();
+    if (!isInt<32>(Offset))
+      return false;
----------------
reames wrote:
> craig.topper wrote:
> > reames wrote:
> > > 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?
> > 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?
> 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?  
> 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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126986