This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Check for LSTP side-effects.
ClosedPublic

Authored by samparker on Sep 24 2020, 3:26 AM.

Details

Summary

If the LSTP instruction is inserted with an element count low enough to immediately predicate some lanes as false, this can have some unintended effects on any proceeding MVE instructions in the preheader.

Diff Detail

Event Timeline

samparker created this revision.Sep 24 2020, 3:26 AM
samparker requested review of this revision.Sep 24 2020, 3:26 AM

Looks good, but ignoring the nits I have one question inlined that asks about explaining why we are doing this, and am interested to have a read first.

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

nit: first thought it was a typo, but it isn't of course....perhaps: LSTP -> [W|D]LSTP

571

can you be a bit more specific, i.e. what the exact conditions are when we can't insert it and why we are doing this?

572

nit: perhaps cannotInsertLSTPBetween -> cannotInsertWDLSTPBetween

samparker updated this revision to Diff 294018.Sep 24 2020, 4:56 AM

Cheers, did some renaming and added the comment.

SjoerdMeijer accepted this revision.Sep 24 2020, 5:13 AM

Thanks, perfectly clear, LGTM.

For bonus points, if you have ideas on that, perhaps a TODO how we can win some of the reverted loops back? Or is it next on your list to look at this?. From a quick look at one of the changed tests, I could be wrong, I see some loopstart instructions sitting in the middle of the block for no good reasons? Is there some low hanging fruit there?

This revision is now accepted and ready to land.Sep 24 2020, 5:13 AM

Thanks. Yes, that's next on my TODO list (why would I want to do anything else?!) What I'd really like to do is simplify the logic and remove all the code that moves stuff around and just place the [D|W]LSTP as the last instruction in the preheader. We'll see how that goes...

Okidoki, nice one

This revision was automatically updated to reflect the committed changes.