This is an archive of the discontinued LLVM Phabricator instance.

[ARM][LowOverheadLoops] Add horizontal reduction support
ClosedPublic

Authored by samparker on Mar 24 2020, 8:16 AM.

Details

Summary

Add a bit more logic into the 'FalseLaneZeros' tracking to enable horizontal reductions and also make the VADDV variants validForTailPredication.

Diff Detail

Event Timeline

samparker created this revision.Mar 24 2020, 8:16 AM
SjoerdMeijer added inline comments.Mar 24 2020, 11:33 AM
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
609

Please add a comment that Unknown is the worklist of instructions that still need to be analysed, or something along those lines.

635

I had to think very hard again why we are returning false here (it's obvious now), but please add a comment to help the reader a bit here.

Or perhaps create a helper "MessingAroundWithTheLanes", which invokes retainsPreviousHalfElement and isHorizontalReduction?

669

I was looking at Unknown again, and was wondering if it would be good to assert that Unknown should be empty at this point here. If I am not mistaken, that's what we expect here? That would require to remove items from Unknown where we insert it into to Predicated at line 667 above, but seems like a good check?

samparker marked 2 inline comments as done.Mar 25 2020, 1:53 AM
samparker added inline comments.
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
609

And I'll now rename it to FalseLaneUnknown.

669

But I'm not sure what value that would add, maybe if the loop lived somewhere else, but the loop can't terminate successfully without that condition being true. I'll add a comment.

samparker updated this revision to Diff 252518.Mar 25 2020, 2:12 AM
  • Bit of renaming.
  • Added some comments.
  • Updated the unit test.
SjoerdMeijer accepted this revision.Mar 25 2020, 3:30 AM

Cheers, nice one.

This revision is now accepted and ready to land.Mar 25 2020, 3:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2020, 2:08 AM