This is an archive of the discontinued LLVM Phabricator instance.

[InterleaveAccess] Recognize Interleave loads through binary operations
ClosedPublic

Authored by dmgreen on Oct 15 2020, 12:02 PM.

Details

Summary

Instcombine will currently sink identical shuffles though vector binary operations. This is probably generally useful, but can break up the code pattern we use to represent an interleaving load group. This patch reverses that in the InterleaveAccessPass to re-recognise the pattern of shuffles sunk past binary operations and folds them back if an interleave group can be created.

Diff Detail

Event Timeline

dmgreen created this revision.Oct 15 2020, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2020, 12:02 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
dmgreen requested review of this revision.Oct 15 2020, 12:02 PM
SjoerdMeijer accepted this revision.Oct 26 2020, 1:10 AM

Nice codegen changes! This patch looks good to me. I haven't looked too much at InterleavedAccessPass though, so perhaps wait a day just in case there are more comments.

This revision is now accepted and ready to land.Oct 26 2020, 1:10 AM

I haven't looked at this pass much, so just pointed out some minors.
There are tests specifically for this pass in test/Transforms/InterleavedAccess/AArch64. So it would be nice to have minimal IR tests using "opt < %s -interleaved-access". If possible, create a test where a load has both shuffle users and shuffle-of-binop users, so we test a scenario where both of those data structures are populated.

llvm/lib/CodeGen/InterleavedAccessPass.cpp
300–305

Should all of these be SmallSetVector?

307

typo: modified

314

auto -> auto *

319

Use auto * here too for consistency.

345–346

for (unsigned i =0, e = Shuffle.size(); i != e; ++i) {

355
dmgreen updated this revision to Diff 300918.Oct 27 2020, 2:15 AM
dmgreen marked 4 inline comments as done.

Added InterleavedAccess and addressed other review comments.

llvm/lib/CodeGen/InterleavedAccessPass.cpp
300–305

I had to remind myself why this was needed. We need to ensure the BinOpShuffles are only replaced once, even if both operands of the binop are the same interleaving load.

spatel accepted this revision.Oct 27 2020, 10:21 AM

Thanks for the cleanups and tests. I still have not stepped through all of the transforms made by this pass, but LGTM.

Thanks for taking a look.