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

[Philip Reames via Phabricator via llvm-commits]

reames 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;
----------------
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?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126986