[PATCH] D54846: [CodeGen][NFC] Make `TII::getMemOpBaseImmOfs` return a base operand

[Nirav Dave via Phabricator via llvm-commits]

niravd added a comment.

Minor nits.



================
Comment at: include/llvm/CodeGen/TargetInstrInfo.h:1141
   /// memory.
-  virtual bool getMemOpBaseRegImmOfs(MachineInstr &MemOp, unsigned &BaseReg,
-                                     int64_t &Offset,
-                                     const TargetRegisterInfo *TRI) const {
+  virtual bool getMemOpBaseImmOfs(MachineInstr &MI, MachineOperand *&BaseOp,
+                                  int64_t &Offset,
----------------
sdesmalen wrote:
> sdesmalen wrote:
> > Is this maybe a good moment to give this a better name, e.g. getMemOperandWithOffset?
> It seems like BaseOp should never be changed, should it be made `const MachineOperand *`?
BaseOp should be a result value so it can't be a result. I think you meant MI which from what I've seen is never touched and should be const.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1138
   const TargetRegisterInfo *TRI = &getRegisterInfo();
-  unsigned BaseRegA = 0, BaseRegB = 0;
+  MachineOperand *BaseOpA = 0, *BaseOpB = 0;
   int64_t OffsetA = 0, OffsetB = 0;
----------------
Use nullptr.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:278
       // Normal, single offset LDS instruction.
-      const MachineOperand *AddrReg =
-          getNamedOperand(LdSt, AMDGPU::OpName::addr);
+      MachineOperand *AddrReg = getNamedOperand(LdSt, AMDGPU::OpName::addr);
 
----------------
AddrReg can be elided into it's use here and the other occurrences later in the file.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:406
+  // Note: this could be extended to support FI operands.
+  if (BaseOp1.getType() != BaseOp2.getType() || !BaseOp1.isReg())
+    return false;
----------------
 "!BaseOp1.isReg() || !BaseOp2.isReg()" would be clearer here.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:414
+
   if (!memOpsHaveSameBasePtr(FirstLdSt, BaseReg1, SecondLdSt, BaseReg2))
     return false;
----------------
These checks should probably be folded up into memOpsHaveSaveBasePtr since it seems to be a complexity encapsulation (static function with a single call). 




================
Comment at: lib/Target/Hexagon/HexagonInstrInfo.cpp:2902
   unsigned AccessSize = 0;
   int OffsetVal = 0;
+  BaseOp = getBaseAndOffset(LdSt, OffsetVal, AccessSize);
----------------
Unrelated but we should elide OffsetVal in favor of directly using Offset here.


================
Comment at: lib/Target/Hexagon/HexagonInstrInfo.cpp:3136
     if (!OffsetOp.isImm())
       return 0;
     Offset = OffsetOp.getImm();
----------------
nullptr


================
Comment at: lib/Target/Hexagon/HexagonInstrInfo.cpp:3142
   if (BaseOp.getSubReg() != 0)
     return 0;
+  return &const_cast<MachineOperand&>(BaseOp);
----------------
nullptr


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

https://reviews.llvm.org/D54846