Page MenuHomePhabricator

[ARM] Use mov operand if the mov cannot be moved while tail predicating
ClosedPublic

Authored by samtebbs on Aug 17 2020, 9:13 AM.

Details

Summary

There are some cases where the instruction that sets up the iteration
count for a tail predicated loop cannot be moved before the dlstp,
stopping tail predication entirely. This patch checks if the mov operand
can be used and if so, uses that instead.

Note for review: I am not sure if my approach to modifying getCount()
and adding TPNumElements is
the best possible approach, please give feedback if you can think of a
better one.

Diff Detail

Event Timeline

samtebbs created this revision.Aug 17 2020, 9:13 AM
samtebbs requested review of this revision.Aug 17 2020, 9:14 AM

I think having the IR test is a good idea, but could you also upload it as a MIR too, just because this pass is very far away from the IR input.

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
230

Maybe just store the Register instead?

295–296

I know it's not your doing, but would you mind renaming this method to something a bit more descriptive?

458

I think just having NumElements as a member of the class would make sense,

489

In ARMBaseInstrInfo.h we have isMovRegOpcode, which also covers the T2 version.

494

Maybe only call ElemDef->getOperand(1) once and give the variable a nice name?

samtebbs added a comment.EditedAug 18 2020, 2:53 AM

I think having the IR test is a good idea, but could you also upload it as a MIR too, just because this pass is very far away from the IR input.

Sure, there is already a mov-after-dls mir test that I can add to.

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
230

I considered that but getCount() needs to return a MachineOperand and I thought that reconstructing a MachineOperand from a Register would be unnecessary.

489

Thanks

494

That makes sense

samtebbs edited the summary of this revision. (Show Details)Aug 18 2020, 3:25 AM
samtebbs edited the summary of this revision. (Show Details)
samtebbs updated this revision to Diff 286268.Aug 18 2020, 6:28 AM
samtebbs marked 3 inline comments as done.

Rename getCount, use is isMovRegOpcode and cache ElemDef->getOperand(1)

Sure, there is already a mov-after-dls mir test that I can add to.

Just generating a MIR file from mov-operand.ll would be fine and save you from manually having to alter an existing test.

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
493

We don't need this variable, right?

495

Bit hard to tell on here, but it doesn't look like we can hit this block anymore.

Sure, there is already a mov-after-dls mir test that I can add to.

Just generating a MIR file from mov-operand.ll would be fine and save you from manually having to alter an existing test.

Yeah that's definitely the best option.

samtebbs added inline comments.Aug 18 2020, 7:46 AM
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
493

TPNumElements? It's used in getLoopStartOperand (previously getCount).

495

Could it not be hit if the if statement above fails?

samtebbs updated this revision to Diff 286281.Aug 18 2020, 7:48 AM

Add MIR test

samparker accepted this revision.Aug 18 2020, 8:06 AM

Thanks! LGTM

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
495

yep! :) I need to learn to collapse comments.

This revision is now accepted and ready to land.Aug 18 2020, 8:06 AM

Thanks! LGTM

Thanks :)

MaskRay added inline comments.
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
1345

Have you tested the patch? getCount is renamed by this patch.

Pushed a fix

samtebbs added inline comments.Aug 19 2020, 1:38 AM
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
1345

Thanks for fixing that. I thought I did build and test after rebasing on master but I must have missed it.