[PATCH] D54846: [CodeGen][NFC] Make `TII::getMemOpBaseImmOfs` return a base operand
Sander de Smalen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 26 05:32:25 PST 2018
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
More information about the llvm-commits
mailing list