This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by thegameg on Nov 23 2018, 2:31 AM.

Details

Summary

Currently, instructions doing memory accesses through a base operand that is not a register can not be analyzed using TII::getMemOpBaseRegImmOfs.

This means that functions such as TII::shouldClusterMemOps will bail out on instructions using an FI as a base instead of a register.

The goal of this patch is to refactor all this to return a base operand instead of a base register.

Then in a separate patch, I will add FI support to the mem op clustering in the MachineScheduler.

Diff Detail

Event Timeline

thegameg created this revision.Nov 23 2018, 2:31 AM

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

include/llvm/CodeGen/TargetInstrInfo.h
1141–1143

Is this maybe a good moment to give this a better name, e.g. getMemOperandWithOffset?

1141–1143

It seems like BaseOp should never be changed, should it be made const MachineOperand *?

lib/CodeGen/MachinePipeliner.cpp
1128

Is there a reason not to use BaseOp1->isIdenticalTo(BaseOp2)?
(other than it possibly making your patch no longer NFC)

lib/Target/AArch64/AArch64InstrInfo.cpp
1156

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.

2250

Do you want to add an assert here that BaseOp is a register?

Minor nits.

include/llvm/CodeGen/TargetInstrInfo.h
1141–1143

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.

lib/Target/AArch64/AArch64InstrInfo.cpp
1138

Use nullptr.

lib/Target/AMDGPU/SIInstrInfo.cpp
278–279

AddrReg can be elided into it's use here and the other occurrences later in the file.

419–422

These checks should probably be folded up into memOpsHaveSaveBasePtr since it seems to be a complexity encapsulation (static function with a single call).

421

"!BaseOp1.isReg() || !BaseOp2.isReg()" would be clearer here.

lib/Target/Hexagon/HexagonInstrInfo.cpp
2900–2901

Unrelated but we should elide OffsetVal in favor of directly using Offset here.

3136

nullptr

3141–3142

nullptr

thegameg updated this revision to Diff 175305.Nov 26 2018, 11:36 AM
thegameg marked 14 inline comments as done.

Addressed some of the reviews.

thegameg marked an inline comment as done.Nov 26 2018, 11:39 AM
thegameg added inline comments.
include/llvm/CodeGen/TargetInstrInfo.h
1141–1143

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.

sdesmalen accepted this revision.Nov 27 2018, 8:28 AM

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

include/llvm/CodeGen/TargetInstrInfo.h
1141–1143

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.

lib/Target/Hexagon/HexagonInstrInfo.cpp
3143

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)

This revision is now accepted and ready to land.Nov 27 2018, 8:28 AM
thegameg marked 2 inline comments as done.Nov 27 2018, 10:20 AM
thegameg added inline comments.
lib/Target/Hexagon/HexagonInstrInfo.cpp
3143

Since this is used to return the base in getMemOperandWithOffset, we would need to make BaseOp const there too. Which brings us to the previous comment.

This revision was automatically updated to reflect the committed changes.
thegameg marked an inline comment as done.

Thanks @sdesmalen, @niravd for the review!