Page MenuHomePhabricator

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

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



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.

1141 ↗(On Diff #175095)

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

1141 ↗(On Diff #175095)

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

1128 ↗(On Diff #175095)

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

1156 ↗(On Diff #175095)

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.

2246 ↗(On Diff #175095)

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

Minor nits.

1141 ↗(On Diff #175095)

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.

1138 ↗(On Diff #175095)

Use nullptr.

278 ↗(On Diff #175095)

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

406 ↗(On Diff #175095)

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

414 ↗(On Diff #175095)

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

2902 ↗(On Diff #175095)

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

3136 ↗(On Diff #175095)


3142 ↗(On Diff #175095)


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.
1141 ↗(On Diff #175095)

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)

1141 ↗(On Diff #175095)

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.

3143 ↗(On Diff #175305)

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.
3143 ↗(On Diff #175305)

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!