This is an archive of the discontinued LLVM Phabricator instance.

[INSTCOMBINE] Transform reduction(shuffle V, poison, unique_mask) to reduction(V).
ClosedPublic

Authored by ABataev on Jun 28 2021, 12:39 PM.

Details

Summary

After SLP + LTO we may have have reduction(shuffle V, poison,
mask). This can be simplified to just reduction(V) if the mask is only
for single vector and just all elements from this vector are permuted,

without reusing, replacing with undefs and/or other values, etc.

Diff Detail

Event Timeline

ABataev created this revision.Jun 28 2021, 12:39 PM
ABataev requested review of this revision.Jun 28 2021, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2021, 12:39 PM

Is this really true for FP reductions? Don't they need reassoc flag for this?

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2000–2001

Not very useful comment

2014

This doesn't deal with non-canonical shuffle variant properly (and the test is missing)

ABataev retitled this revision from [INSTCOMBINE] Tramsform reduction(shuffle V, poison, unique_mask) to reduction(V). to [INSTCOMBINE] Transform reduction(shuffle V, poison, unique_mask) to reduction(V)..Jun 28 2021, 1:10 PM

Is this really true for FP reductions? Don't they need reassoc flag for this?

I assume you're right, will add a check.

ABataev added inline comments.Jun 28 2021, 1:19 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2014

What do you mean by non-canonical shuffle?

lebedev.ri added inline comments.Jun 28 2021, 1:22 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2014

match(Arg, m_Shuffle(m_Undef(), m_Value(V), m_Mask(Mask)))

ABataev added inline comments.Jun 28 2021, 1:27 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2014

Ah, yes, will fix it, thanks!

ABataev updated this revision to Diff 355020.Jun 28 2021, 1:33 PM

Address comments.

spatel added inline comments.Jun 29 2021, 5:08 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2013

We canonicalize undef/poison operand as the 2nd input vector and we're within InstCombine, so checking for that is unnecessary?

https://github.com/llvm/llvm-project/blob/b458bb8c04cd5ed025884d424f386a00c9c6857e/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp#L2427

The tests have coverage now to verify that unless I missed something in the earlier draft.

llvm/test/Transforms/InstCombine/reduction-shufflevector.ll
10

Identity shuffle should be reduced even without this patch.
It would be better to pre-commit the test file, so we can make sure that we are testing as expected.

159

It's independent of this patch, but we might want to mark this and similar tests with a TODO - if a reduction has a known undef/poison element in the input, the result can be simplified.

ABataev added inline comments.Jun 29 2021, 5:11 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2013

Ok, will check it and remove this extra check.

llvm/test/Transforms/InstCombine/reduction-shufflevector.ll
10

Yes, I believe so, just added a check to try all possible combinations.
Did not precommit the test initially to be sure that the patch/the test are correct. Will precommit it, if it is fine.

159

Yes, thought about it too, just forgot to add it, thanks!

spatel added inline comments.Jun 29 2021, 5:20 AM
llvm/test/Transforms/InstCombine/reduction-shufflevector.ll
10

Sure, please precommit - the transform can be confirmed by Alive2:
https://alive2.llvm.org/ce/z/CZH3eH

ABataev updated this revision to Diff 355188.Jun 29 2021, 5:35 AM

Rebase + remove the check for non-canonical shuffle.

spatel accepted this revision.Jun 29 2021, 7:00 AM

LGTM - see inline for a few minor points.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2010

Nit: we're not using FVTy, so it would be easier to read (less indented) if we just bail out on scalable vectors:

if (!isa<FixedVectorType>(Arg->getType())) break;

You could also combine the next set of conditions with this to early exit and remove another level of indent.

2015

Unusual spelling: Indices or Indexes?

llvm/test/Transforms/InstCombine/reduction-shufflevector.ll
90

This is correct (no FMF required) based on LangRef, but it would be good to add some extra flags on at least one FP test, so we verify that those are all propagated through the transform.

This revision is now accepted and ready to land.Jun 29 2021, 7:00 AM