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.
Details
- Reviewers
dmgreen SjoerdMeijer - Commits
- rG42350cd893a9: [ARM][MVE] Tail Predicate IsSafeToRemove
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
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 |
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. |
llvm/test/CodeGen/Thumb2/LowOverheadLoops/wlstp.mir | ||
---|---|---|
365 | And the other is in vctp-subi3.mir. |
Thanks, LGTM
llvm/test/CodeGen/Thumb2/LowOverheadLoops/wlstp.mir | ||
---|---|---|
365 | ah, missed that one! |
typo: the the