This is an archive of the discontinued LLVM Phabricator instance.

[ARM][MVE] Tail Predicate IsSafeToRemove
ClosedPublic

Authored by samparker on Dec 23 2019, 5:33 AM.

Details

Summary

Introduce a method to walk through use-def chains to decide whether it's possible to remove a given instruction and its users. These instructions are then stored in a set until the end of the transform when they're erased. This is now used to perform checks on the iteration count (LoopDec chain) and element count (VCTP chain).
As well as being able to remove chains of instructions, we know also check that the sub feeding the vctp is producing the expected value. The checks around the vctp instruction have been moved into their own method 'ValidateTailPredicate' which has made this change look bigger than it is.
The test changes have couple of tweaks to ensure that the test still works. But I've also added tests for sub -> vctp chains as well as another for completing removing redundant movs in the loop.

Diff Detail

Event Timeline

samparker created this revision.Dec 23 2019, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2019, 5:33 AM
samparker updated this revision to Diff 236005.Jan 3 2020, 2:09 AM

Rebased on top of master and D72131.

samparker edited the summary of this revision. (Show Details)Jan 3 2020, 2:11 AM
samparker edited the summary of this revision. (Show Details)
SjoerdMeijer added inline comments.Jan 6 2020, 3:03 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.h
660

What about tSUBSi3, do we need to consider that too?

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

style: personally I think getLocalUses could be a local static helper function. It is a lambda, but pretty big, and only invoked here. I.e. it isn't e.g. used as an object that is passed to an algorithm.

394

typo: if -> is

514–562

I wanted to comment if you could be more specific what you mean with value change and what we expect here, but reading a bit further clarified that, but perhaps....

518

...renaming VecWidth to ExpectedVecWidth or something along those lines help a bit.

520

Is this also a little helper function that could live in ARMBaseInstrInfo as e.g. getSubImmOpIndex?

samparker updated this revision to Diff 236787.Jan 8 2020, 3:26 AM

A couple of changes to RDA:

  • It how has getLiveInUses which collects all the users of a live-in register.
  • It now has getAllUses, which collects locals users and users in the successor blocks (using getLiveInUses).

IsSafeToRemove can now use getAllUses to calculate whether an instruction can be removed and it allows us to span an arbitary CFG, including loops. This means we can now use IsSafeToRemove to remove the redundant iteration count calculate when we're using tail predication.

A round of nits:

llvm/include/llvm/CodeGen/ReachingDefAnalysis.h
137

typo: the the

llvm/lib/CodeGen/ReachingDefAnalysis.cpp
262

How about getReachingGlobalUses() because it is more consistent with getReachingLocalUses() that we already have and also more explicit about its "scope" of all uses.

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

nit: perhaps a debug message here that MI has side-effects and can't be removed

895–926

nit: MachineInstr* A... -> MachineInstr *A...

900

I was wondering if it was a bit counter intuitive to start this ExpandLoopStart function with removing loop iteration instructions. We have RemoveDeadBranch that is called after ExpandLoopStart, so perhaps a similar helper function for this?

903

nit: indentation

samparker marked an inline comment as done.Jan 17 2020, 12:50 AM
samparker added inline comments.
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
900

I think removing the instruction, as well as its operand, in the same place makes sense. It's a bit different to the RemoveDeadBranch because that's really just a hack around the lack of understanding of LEUpdate in the backend.

Rebased, cleaned up some tests and addressed comments.

Removed some accidentally added files.

SjoerdMeijer added inline comments.Jan 17 2020, 2:01 AM
llvm/test/CodeGen/Thumb2/LowOverheadLoops/vctp-subri.mir
147

are we testing t2subri3 here?

llvm/test/CodeGen/Thumb2/LowOverheadLoops/wlstp.mir
365

Ah, here's tSUBi8.

samparker marked an inline comment as done.Jan 17 2020, 5:04 AM
samparker added inline comments.
llvm/test/CodeGen/Thumb2/LowOverheadLoops/wlstp.mir
365

And the other is in vctp-subi3.mir.

SjoerdMeijer accepted this revision.Jan 17 2020, 5:06 AM

Thanks, LGTM

llvm/test/CodeGen/Thumb2/LowOverheadLoops/wlstp.mir
365

ah, missed that one!

This revision is now accepted and ready to land.Jan 17 2020, 5:06 AM
This revision was automatically updated to reflect the committed changes.