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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
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))) {
----------------
reames wrote:
> 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.
I wrote the patch to use SelectionDAG::isBaseWithConstantOffset and it does cause test changes to vararg.ll.


================
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)>;
----------------
reames wrote:
> 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.  
ComplexPatterns can't nest so AddRegImm AddrFI:$Rs, simm12:$imm12 wouldn't work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126932