This is an archive of the discontinued LLVM Phabricator instance.

[SLP][NFC]Inital redesign of ShuffleInstructionBuilder, NFC.
ClosedPublic

Authored by ABataev on Dec 9 2022, 7:50 AM.

Details

Summary

The patch redesigns ShuffleInstructionBuilder so it could later be used
for reshuffling of the buildvector sequences and vectorized parts of
externally used scalars. Also will allow to generalize cost model for
the gathers/buildvectors.

Part of D110978.

Diff Detail

Event Timeline

ABataev created this revision.Dec 9 2022, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2022, 7:50 AM
ABataev requested review of this revision.Dec 9 2022, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2022, 7:50 AM
lebedev.ri edited the summary of this revision. (Show Details)Dec 9 2022, 8:26 AM
vdmitrie added inline comments.Dec 9 2022, 9:35 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
8119

Could you please add more details on restrictions assumed on masks and input vectors? I mean sizes of masks and vectors.
The description will greatly benefit from an example.

8153

Please describe the purpose of CommonMask and InVectors data?

8171

description?

8179

description?

8215

this code is repeating. Seems like it makes sense to put additional efforts to cleanup it a bit. Can we sink it past the if-else blocks and fuse with loop at 8249?

ABataev added inline comments.Dec 9 2022, 9:44 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
8119

There will be no limitations on masks and vectors. What kind of example do you think might be useful here?

8153

Ok

8171

Ok

8179

OK

8215

I think it will be much harder to understand this complex code, if these 2 loops are fused. I'll try to outline common part to lambdas/functions.

vdmitrie added inline comments.Dec 9 2022, 10:03 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
8215

okay, no worries. leave it as is so far

vdmitrie added inline comments.Dec 9 2022, 10:12 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
8119

There will be no limitations on masks and vectors. What kind of example do you think might be useful here?

For example, can size of mask be less than size of vectors? This class is supposed to be used to merge two vectors of the same size (for alternating ops) and/or extend a vector (i.e. replicate some of the elements). Right? Here in SLP vectorizer there are not so many uses cases that the class handles yet. So I'm asking to list these cases explicitly.

ABataev added inline comments.Dec 9 2022, 10:14 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
8119

Yes, it will do it later. It will resize the operands, if required.
I'll try to add some examples.

ABataev updated this revision to Diff 481697.Dec 9 2022, 11:03 AM

Address comments

vdmitrie added inline comments.Dec 12 2022, 6:34 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
8141–8147

Hm, it looks like there is a mistake in the second example.

`Lets assume %0 -> <x0, x1>
Lets assume %1 -> <y0, y1>
Then these instructions
%s1 = shufflevector <2 x <ty>> %0, poison, <1, 0, 1, 0>
%s2 = shufflevector <2 x <ty>> %1, poison, <1, 0, 1, 0>

yield

%s1 : <x1, x0, x1, x0>
%s2 : <y1, y0, y1, y0>

Then for
%res = shufflevector <4 x <ty>> %s1, <4 x <ty>> %s2, <1, 0, 4, 3>

<x1, x0, x1, x0><y1, y0, y1, y0>
i,e %res will be <x0, x1 ,y1, x0>
equivalent to
%res = shufflevector <2 x <ty>> %0, <2 x <ty>> %1, <0, 1, 3, 0>

It looks like second pair is off by one, i.e. <1, 0, 4, 3> in the example should actually be <1, 0, 5, 4> (or <1, 0, 7, 6>) in order <0,1,2,3> to yield the same result from shuffling the original operands. Right?
`
btw nit: I suggest to change spelling slightly to either look similar to LangRef: <2 x <ty>> or just <2 x ty>
+ there are few places with extra space.

8316

This actually looks like a catch. Subtle difference between two methods like their arguments ArrayRef<int> vs ArrayRef<unsigned> actually is not so obvious to imply different semantics. I'd suggest to also name the method differently (basically like it was named earlier).

8553

nit: IMO for better readability it makes sense to add "TreeEntry *E" explicitly as argument.

ABataev updated this revision to Diff 482469.Dec 13 2022, 7:21 AM

Address comments, rebase

vdmitrie accepted this revision.Dec 13 2022, 9:22 AM

Looks good. Thanks!

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
8128–8129

nit: space after '1'

8141–8142

nit: space after '1'

This revision is now accepted and ready to land.Dec 13 2022, 9:22 AM
This revision was landed with ongoing or failed builds.Dec 13 2022, 9:54 AM
This revision was automatically updated to reflect the committed changes.