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

[Sander de Smalen via Phabricator via llvm-commits]

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

The patch looks fine to me (with nit on the const_cast)



================
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,
----------------
thegameg wrote:
> niravd wrote:
> > 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.
> It was my first intention to do so, but a lot of APIs are not properly "constified" so I had to keep this non-const. I am planning to do that cleanup for everything in a future patch, though. Let me know if you think it's important to have it now.
> 
> The `MachineOperand` could be const (`const MachineOperand *&BaseOp`). In this case, what can't be const is the pointer (`MachineOperand *const &BaseOp`).
> 
> Regarding the MI, I decided not to do it in this patch for the same reasons above.
Ah yes, I meant to write `const MachineOperand *&BaseOp`.
I'm happy if you want to do that in a separate patch for transitive const-ness reasons.


================
Comment at: lib/Target/Hexagon/HexagonInstrInfo.cpp:3143
+    return nullptr;
+  return &const_cast<MachineOperand&>(BaseOp);
 }
----------------
Maybe I missed something in the uses of this function, but I think you can just return a `const MachineOperand *` here? (thus not needing the const_cast)


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

https://reviews.llvm.org/D54846