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

[craig.topper]

Summary

Previously we had 3 different isel patterns for every scalar load
store instruction.

This reduces them to a single ComplexPattern that returns the Base
and Offset. Or an offset of 0 if there was no offset identified

I've done a similar thing for the 2 isel patterns that match add/or
with FrameIndex and immediate. Using the offset of 0, I was also
able to remove the custom handler for FrameIndex. Happy to split that
to another patch.

We might be able to enhance in the future to remove the post-isel
peephole or the special handling for ADD with constant added by D126576.

A nice side effect is that this removes nearly 3000 bytes from the isel
table.

[reames]

This looks clearly better to me. A couple small comments, but plan to LGTM after that.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

1866

Minor: You can combine these two blocks if written as:
if (Addr.getOpcode() == ISD::ADD ||

(Addr.getOpcode() == ISD::OR && isOrEquivalentToAdd(Addr.getNode()))

1897

Same minor code structure comment as above.

Given we repeat this, maybe add a stripAndAccumulate12BitOffset helper?

1908

Why does this case need to be restricted to frame index? I think you maybe copy pasted this and didn't adjust?

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

1908

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.

[reames]

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

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

1908

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.

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1207–1208

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.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

1908

I wrote the patch to use SelectionDAG::isBaseWithConstantOffset and it does cause test changes to vararg.ll.

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1207–1208

ComplexPatterns can't nest so AddRegImm AddrFI:$Rs, simm12:$imm12 wouldn't work.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.h

public:
   void PreprocessISelDAG() override;
   void PostprocessISelDAG() override;
 
   void Select(SDNode *Node) override;
 
   bool SelectInlineAsmMemoryOperand(const SDValue &Op, unsigned ConstraintID,
                                     std::vector<SDValue> &OutOps) override;
 
-  bool SelectAddrFI(SDValue Addr, SDValue &Base);
+  bool SelectFrameAddrRegImm(SDValue Addr, SDValue &Base, SDValue &Offset);
   bool SelectBaseAddr(SDValue Addr, SDValue &Base);
+  bool SelectAddrRegImm(SDValue Addr, SDValue &Base, SDValue &Offset);
 
   bool selectShiftMask(SDValue N, unsigned ShiftWidth, SDValue &ShAmt);
   bool selectShiftMaskXLen(SDValue N, SDValue &ShAmt) {
     return selectShiftMask(N, Subtarget->getXLen(), ShAmt);
   }
   bool selectShiftMask32(SDValue N, SDValue &ShAmt) {
     return selectShiftMask(N, 32, ShAmt);
   }

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

case ISD::Constant: {
     // If the upper 32-bits are not used try to convert this into a simm32 by
     // sign extending bit 32.
     if (!isInt<32>(Imm) && isUInt<32>(Imm) && hasAllWUsers(Node))
       Imm = SignExtend64<32>(Imm);
 
     ReplaceNode(Node, selectImm(CurDAG, DL, VT, Imm, *Subtarget));
     return;
   }
-  case ISD::FrameIndex: {
-    SDValue Imm = CurDAG->getTargetConstant(0, DL, XLenVT);
-    int FI = cast<FrameIndexSDNode>(Node)->getIndex();
-    SDValue TFI = CurDAG->getTargetFrameIndex(FI, VT);
-    ReplaceNode(Node, CurDAG->getMachineNode(RISCV::ADDI, DL, VT, TFI, Imm));
-    return;
-  }
   case ISD::ADD: {
     // Try to select ADD + immediate used as memory addresses to
     // (ADDI (ADD X, Imm-Lo12), Lo12) if it will allow the ADDI to be removed by
     // doPeepholeLoadStoreADDI.
 
     // LHS should be an immediate.
     auto *N1C = dyn_cast<ConstantSDNode>(Node->getOperand(1));
     if (!N1C)
case InlineAsm::Constraint_A:
     return false;
   default:
     break;
   }
 
   return true;
 }
 
-bool RISCVDAGToDAGISel::SelectAddrFI(SDValue Addr, SDValue &Base) {
+// Select a frame index and an optional immediate offset from an ADD or OR.
+bool RISCVDAGToDAGISel::SelectFrameAddrRegImm(SDValue Addr, SDValue &Base,
+                                              SDValue &Offset) {
   if (auto *FIN = dyn_cast<FrameIndexSDNode>(Addr)) {
     Base = CurDAG->getTargetFrameIndex(FIN->getIndex(), Subtarget->getXLenVT());
+    Offset = CurDAG->getTargetConstant(0, SDLoc(Addr), Subtarget->getXLenVT());
+    return true;
+  }
+
+  // TODO: Use SelectionDAG::isBaseWithConstantOffset.
+  if (Addr.getOpcode() == ISD::ADD ||
+      (Addr.getOpcode() == ISD::OR && isOrEquivalentToAdd(Addr.getNode()))) {
+    if (auto *FIN = dyn_cast<FrameIndexSDNode>(Addr.getOperand(0))) {
+      if (auto *CN = dyn_cast<ConstantSDNode>(Addr.getOperand(1))) {
+        if (isInt<12>(CN->getSExtValue())) {
+          Base = CurDAG->getTargetFrameIndex(FIN->getIndex(),
+                                             Subtarget->getXLenVT());
+          Offset = CurDAG->getTargetConstant(CN->getSExtValue(), SDLoc(Addr),
+                                             Subtarget->getXLenVT());
           return true;
         }
+      }
+    }
+  }
+
   return false;
 }
 
 bool RISCVDAGToDAGISel::SelectBaseAddr(SDValue Addr, SDValue &Base) {
   // If this is FrameIndex, select it directly. Otherwise just let it get
   // selected to a register independently.
   if (auto *FIN = dyn_cast<FrameIndexSDNode>(Addr))
     Base = CurDAG->getTargetFrameIndex(FIN->getIndex(), Subtarget->getXLenVT());
   else
     Base = Addr;
   return true;
 }
 
+bool RISCVDAGToDAGISel::SelectAddrRegImm(SDValue Addr, SDValue &Base,
+                                         SDValue &Offset) {
+  if (Addr.getOpcode() == ISD::ADD) {
+    if (auto *CN = dyn_cast<ConstantSDNode>(Addr.getOperand(1))) {
+      if (isInt<12>(CN->getSExtValue())) {
+        SelectBaseAddr(Addr.getOperand(0), Base);
+        Offset = CurDAG->getTargetConstant(CN->getSExtValue(), SDLoc(Addr),
+                                           Subtarget->getXLenVT());
+        return true;
+      }
+    }
+  } else if (Addr.getOpcode() == ISD::OR) {
+    // We might be able to treat this OR as an ADD.
+    // TODO: Use SelectionDAG::isBaseWithConstantOffset.
+    if (auto *FIN = dyn_cast<FrameIndexSDNode>(Addr.getOperand(0))) {
+      if (auto *CN = dyn_cast<ConstantSDNode>(Addr.getOperand(1))) {
+        if (isInt<12>(CN->getSExtValue()) &&
+            isOrEquivalentToAdd(Addr.getNode())) {
+          Base = CurDAG->getTargetFrameIndex(FIN->getIndex(),
+                                             Subtarget->getXLenVT());
+          Offset = CurDAG->getTargetConstant(CN->getSExtValue(), SDLoc(Addr),
+                                             Subtarget->getXLenVT());
+          return true;
+        }
+      }
+    }
+  }
+
+  Offset = CurDAG->getTargetConstant(0, SDLoc(Addr), Subtarget->getXLenVT());
+  return SelectBaseAddr(Addr, Base);
+}
+
 bool RISCVDAGToDAGISel::selectShiftMask(SDValue N, unsigned ShiftWidth,
                                         SDValue &ShAmt) {
   // Shift instructions on RISCV only read the lower 5 or 6 bits of the shift
   // amount. If there is an AND on the shift amount, we can bypass it if it
   // doesn't affect any of those bits.
   if (N.getOpcode() == ISD::AND && isa<ConstantSDNode>(N.getOperand(1))) {
     const APInt &AndMask = N->getConstantOperandAPInt(1);
 

llvm/lib/Target/RISCV/RISCVInstrInfo.td

 
 // A 6-bit constant greater than 32.
 def uimm6gt32 : ImmLeaf<XLenVT, [{
   return isUInt<6>(Imm) && Imm > 32;
 }]>;
 
 // Addressing modes.
 // Necessary because a frameindex can't be matched directly in a pattern.
-def AddrFI : ComplexPattern<iPTR, 1, "SelectAddrFI">;
+def FrameAddrRegImm : ComplexPattern<iPTR, 2, "SelectFrameAddrRegImm",
+                                     [frameindex, or, add]>;
 def BaseAddr : ComplexPattern<iPTR, 1, "SelectBaseAddr">;
+def AddrRegImm : ComplexPattern<iPTR, 2, "SelectAddrRegImm">;
 
 // Return the negation of an immediate value.
 def NegImm : SDNodeXForm<imm, [{
   return CurDAG->getTargetConstant(-N->getSExtValue(), SDLoc(N),
                                    N->getValueType(0));
 }]>;
 
 // Return an immediate value minus 32.
: Pat<(XLenVT (OpNode (XLenVT GPR:$rs1), ImmType:$imm)),
           (Inst GPR:$rs1, ImmType:$imm)>;
 class PatGprSimm12<SDPatternOperator OpNode, RVInstI Inst>
     : PatGprImm<OpNode, Inst, simm12>;
 class PatGprUimmLog2XLen<SDPatternOperator OpNode, RVInstIShift Inst>
     : PatGprImm<OpNode, Inst, uimmlog2xlen>;
 
 /// Predicates
 
-def IsOrAdd: PatFrag<(ops node:$A, node:$B), (or node:$A, node:$B), [{
-  return isOrEquivalentToAdd(N);
-}]>;
 def assertsexti32 : PatFrag<(ops node:$src), (assertsext node:$src), [{
   return cast<VTSDNode>(N->getOperand(1))->getVT().bitsLE(MVT::i32);
 }]>;
 def sexti32 : ComplexPattern<i64, 1, "selectSExti32">;
 def assertzexti32 : PatFrag<(ops node:$src), (assertzext node:$src), [{
   return cast<VTSDNode>(N->getOperand(1))->getVT().bitsLE(MVT::i32);
 }]>;
 def zexti32 : ComplexPattern<i64, 1, "selectZExti32">;

 // as a hint to the linker.
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0, isCodeGenOnly = 0 in
 def PseudoAddTPRel : Pseudo<(outs GPR:$rd),
                             (ins GPR:$rs1, GPR:$rs2, tprel_add_symbol:$src), [],
                             "add", "$rd, $rs1, $rs2, $src">;
 
 /// FrameIndex calculations
 
-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)>;
 
 /// Setcc
 
 def : PatGprGpr<setlt, SLT>;
 def : PatGprSimm12<setlt, SLTI>;
 def : PatGprGpr<setult, SLTU>;
 def : PatGprSimm12<setult, SLTIU>;
 

 let Predicates = [IsRV64], hasSideEffects = 0, mayLoad = 0, mayStore = 0,
   isCodeGenOnly = 0, isAsmParserOnly = 1 in {
 def PseudoZEXT_W : Pseudo<(outs GPR:$rd), (ins GPR:$rs), [], "zext.w", "$rd, $rs">;
 } // Predicates = [IsRV64], ...
 
 /// Loads
 
 multiclass LdPat<PatFrag LoadOp, RVInst Inst, ValueType vt = XLenVT> {
-  def : Pat<(vt (LoadOp BaseAddr:$rs1)), (Inst BaseAddr:$rs1, 0)>;
-  def : Pat<(vt (LoadOp (add BaseAddr:$rs1, simm12:$imm12))),
-            (Inst BaseAddr:$rs1, simm12:$imm12)>;
-  def : Pat<(vt (LoadOp (IsOrAdd AddrFI:$rs1, simm12:$imm12))),
-            (Inst AddrFI:$rs1, simm12:$imm12)>;
+  def : Pat<(vt (LoadOp (AddrRegImm GPR:$rs1, simm12:$imm12))),
+            (Inst GPR:$rs1, simm12:$imm12)>;
 }
 
 defm : LdPat<sextloadi8, LB>;
 defm : LdPat<extloadi8, LB>;
 defm : LdPat<sextloadi16, LH>;
 defm : LdPat<extloadi16, LH>;
 defm : LdPat<load, LW, i32>, Requires<[IsRV32]>;
 defm : LdPat<zextloadi8, LBU>;
 defm : LdPat<zextloadi16, LHU>;
 
 /// Stores
 
 multiclass StPat<PatFrag StoreOp, RVInst Inst, RegisterClass StTy,
                  ValueType vt> {
-  def : Pat<(StoreOp (vt StTy:$rs2), BaseAddr:$rs1),
-            (Inst StTy:$rs2, BaseAddr:$rs1, 0)>;
-  def : Pat<(StoreOp (vt StTy:$rs2), (add BaseAddr:$rs1, simm12:$imm12)),
-            (Inst StTy:$rs2, BaseAddr:$rs1, simm12:$imm12)>;
-  def : Pat<(StoreOp (vt StTy:$rs2), (IsOrAdd AddrFI:$rs1, simm12:$imm12)),
-            (Inst StTy:$rs2, AddrFI:$rs1, simm12:$imm12)>;
+  def : Pat<(StoreOp (vt StTy:$rs2), (AddrRegImm GPR:$rs1, simm12:$imm12)),
+            (Inst StTy:$rs2, GPR:$rs1, simm12:$imm12)>;
 }
 
 defm : StPat<truncstorei8, SB, GPR, XLenVT>;
 defm : StPat<truncstorei16, SH, GPR, XLenVT>;
 defm : StPat<store, SW, GPR, i32>, Requires<[IsRV32]>;
 
 /// Fences
 

llvm/lib/Target/RISCV/RISCVInstrInfoA.td

multiclass AMO_rr_aq_rl<bits<5> funct5, bits<3> funct3, string opcodestr> {
   def ""     : AMO_rr<funct5, 0, 0, funct3, opcodestr>;
   def _AQ    : AMO_rr<funct5, 1, 0, funct3, opcodestr # ".aq">;
   def _RL    : AMO_rr<funct5, 0, 1, funct3, opcodestr # ".rl">;
   def _AQ_RL : AMO_rr<funct5, 1, 1, funct3, opcodestr # ".aqrl">;
 }
 
 multiclass AtomicStPat<PatFrag StoreOp, RVInst Inst, RegisterClass StTy,
                        ValueType vt = XLenVT> {
-  def : Pat<(StoreOp BaseAddr:$rs1, (vt StTy:$rs2)),
-            (Inst StTy:$rs2, BaseAddr:$rs1, 0)>;
-  def : Pat<(StoreOp (add BaseAddr:$rs1, simm12:$imm12), (vt StTy:$rs2)),
-            (Inst StTy:$rs2, BaseAddr:$rs1, simm12:$imm12)>;
-  def : Pat<(StoreOp (IsOrAdd AddrFI:$rs1, simm12:$imm12), (vt StTy:$rs2)),
-            (Inst StTy:$rs2, AddrFI:$rs1, simm12:$imm12)>;
+  def : Pat<(StoreOp (AddrRegImm GPR:$rs1, simm12:$imm12), (vt StTy:$rs2)),
+            (Inst StTy:$rs2, GPR:$rs1, simm12:$imm12)>;
 }
 
 //===----------------------------------------------------------------------===//
 // Instructions
 //===----------------------------------------------------------------------===//
 
 let Predicates = [HasStdExtA] in {
 defm LR_W       : LR_r_aq_rl<0b010, "lr.w">, Sched<[WriteAtomicLDW, ReadAtomicLDW]>;