[PATCH] D126932: [RISCV] Reduce scalar load/store isel patterns to a single ComplexPattern. NFCI

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/minor comments addressed.  I'm also fine if a couple get split into following patches at your discretion.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:1920
+    // We might be able to treat this OR as an ADD.
+    if (auto *FIN = dyn_cast<FrameIndexSDNode>(Addr.getOperand(0))) {
+      if (auto *CN = dyn_cast<ConstantSDNode>(Addr.getOperand(1))) {
----------------
craig.topper wrote:
> reames wrote:
> > Why does this case need to be restricted to frame index?  I think you maybe copy pasted this and didn't adjust?
> This was an attempt to preserve the original isel pattern. isOrEquivalentToAdd only handles frame index and the isel pattern used AddrFI.
> 
> I'm considering using the more powerful SelectionDAG::isBaseWithConstantOffset that uses computeKnownBits for OR instead of special casing FrameIndex.
Let's do that as a follow up.  That way the code changes end up in their own review separate from what is otherwise mostly NFC.

Add a comment to that effect in this patch please.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1207
 
-def : Pat<(add (XLenVT AddrFI:$Rs), simm12:$imm12),
-          (ADDI (XLenVT AddrFI:$Rs), simm12:$imm12)>;
-def : Pat<(IsOrAdd (XLenVT AddrFI:$Rs), simm12:$imm12),
-          (ADDI (XLenVT AddrFI:$Rs), simm12:$imm12)>;
+def : Pat<(FrameAddrRegImm GPR:$rs1, simm12:$imm12),
+          (ADDI GPR:$rs1, simm12:$imm12)>;
----------------
Do we even need a separate FrameAddRegImm here?  The addressing mode should be the same for FI vs non-FI.

Can we write something along the lines of AddRegImm  AddrFI:$Rs, simm12:$imm12?  Not sure of my tablegen syntax here.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126932