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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for making this change, I think having the interface more generic makes sense. Please find some comments inlined.



================
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,
----------------
Is this maybe a good moment to give this a better name, e.g. getMemOperandWithOffset?


================
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:
> 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 *`?


================
Comment at: lib/CodeGen/MachinePipeliner.cpp:1128
+              TII->getMemOpBaseImmOfs(MI, BaseOp2, Offset2, TRI)) {
+            if (BaseOp1->isReg() && BaseOp2->isReg() &&
+                BaseOp1->getReg() == BaseOp2->getReg() &&
----------------
Is there a reason not to use BaseOp1->isIdenticalTo(BaseOp2)?
(other than it possibly making your patch no longer NFC)


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1156
+      getMemOpBaseImmOfsWidth(MIb, BaseOpB, OffsetB, WidthB, TRI)) {
+    if ((BaseOpA->isReg() && BaseOpB->isReg() &&
+         BaseOpA->getReg() == BaseOpB->getReg())) {
----------------
Similar to my question above (and perhaps more a question for your other patch, D54847) if this could just be 'BaseOpA->isIdenticalTo(BaseopB)'. If you add the assert to getMemOpBaseImmOfsWidth that the base will always be either a register or FI, this check becomes simpler.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2246
   }
   return true;
 }
----------------
Do you want to add an assert here that BaseOp is a register?


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

https://reviews.llvm.org/D54846