This is an archive of the discontinued LLVM Phabricator instance.

[SLP]Initial support for reshuffling of non-starting buildvector/gather nodes.
ClosedPublic

Authored by ABataev on Feb 28 2023, 5:21 AM.

Details

Summary

Previously only the very first gather/buildvector node might be probed for reshuffling of other nodes.
But the compiler may do the same for other gather/buildvector nodes too, just need to check the
dependency and postpone the emission of the dependent nodes, if the origin nodes were not emitted yet.

Part of D110978

Diff Detail

Event Timeline

ABataev created this revision.Feb 28 2023, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 5:21 AM
ABataev requested review of this revision.Feb 28 2023, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 5:21 AM
vdmitrie added inline comments.Feb 28 2023, 6:12 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9440

nit:
These

unsigned VF1 = cast<FixedVectorType>(V1->getType())->getNumElements();
unsigned VF2 = cast<FixedVectorType>(V2->getType())->getNumElements();

can be hoisted above if at line 9412 and VF1 then reused here.

the "if" condition at 9412 can be then changed into "VF1 != VF2"

9455

nit: It looks like optional for return type isn't really needed here. Returning Value* type and nullptr instead of std::nullopt should do the trick (assuming appropriate changes at call site)

llvm/test/Transforms/SLPVectorizer/X86/PR35865.ll
12

Hm, this looks weird that now SLP vectorizer works like dead code elimination pass. May be update the test so that it would not be dead code instead?

ABataev updated this revision to Diff 501519.Mar 1 2023, 8:03 AM

Address comments

RKSimon added inline comments.Mar 2 2023, 5:19 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9420

Should VF be std::max(VF1, VF2) ?

ABataev added inline comments.Mar 2 2023, 5:49 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9420

Yes, thanks!

ABataev updated this revision to Diff 501925.Mar 2 2023, 10:57 AM

Address comments, allow to shuffle nodes with different VF.

RKSimon added inline comments.Mar 3 2023, 3:11 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9418

unsigned VF = std::max(VF1, VF2);

9420

Drop this?

9505

Use braces here as the multi-line if() is difficult to keep track of

9564

Should we have done this already as another patch? A lot of the test changes seem related to this.

llvm/test/Transforms/SLPVectorizer/X86/PR35865-inseltpoison.ll
12

Update the test IR to match PR35865.ll (just with poison instead of undef)

ABataev added inline comments.Mar 3 2023, 6:42 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9564

Previous patches do not require this change, it is required only starting with this patch unfortunatelly.

ABataev updated this revision to Diff 502121.Mar 3 2023, 6:45 AM

Address comments

RKSimon accepted this revision.Mar 7 2023, 8:07 AM

LGTM - cheers

This revision is now accepted and ready to land.Mar 7 2023, 8:07 AM
vdmitrie added inline comments.Mar 7 2023, 9:12 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6998–6999

What is a case when count(VL, *It) == 1?

ABataev added inline comments.Mar 7 2023, 9:14 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6998–6999

It's when you have something like <poison, poison, v, poison>.

LG. Thanks.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6998–6999

Thanks. I see now. I did not realize that isSplat returns true for even single non-undef value in a vector.

RKSimon added inline comments.Mar 7 2023, 9:37 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6998–6999

Yes - its a problem in both IR and DAG :(

This revision was landed with ongoing or failed builds.Mar 7 2023, 12:47 PM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Mar 10 2023, 5:40 AM

This caused verifier errors / register allocation crashes in Chromium. See https://bugs.chromium.org/p/chromium/issues/detail?id=1423355 for a reproducer.

Reverting until that can be fixed.

bgraur added a subscriber: bgraur.Mar 10 2023, 5:48 AM

We've found another clang crasher caused by this change.

repro.c:

double a = 0, b = 0, c = 0, d = 0, e = 0, f = 0, g = 0, h = 0, i = 0, j = 0;
void n() {
  double k, l, m = g = h = j = e;
  k = e * e;
  l = 0.3e1 * k - e + e - k - g + 0.7e1 * e * 0 * h - i + 0.6e1 * e * 0 * j;
  m = 0 * e + 0.68e2 * e * 0 * e - a + 0.57e2 * e * 0 * b - e +
      0.2e1 * e * 0 * e;
  c = f ? d + l + m : 0;
}

Compilation command:

clang -cc1 -triple x86_64-pc-linux-gnu -emit-obj \
  -O2 -vectorize-slp \
  -x c repro.c \
  -o /tmp/obj

Output:

Invalid shufflevector operands!
  %11 = shufflevector <2 x double> %5, PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: bin/clang -cc1 -triple x86_64-pc-linux-gnu -emit-obj -O2 -vectorize-slp -x c repro.c -o /tmp/obj
1.      <eof> parser at end of file
2.      Code generation
3.      Running pass 'Function Pass Manager' on module 'repro.c'.
4.      Running pass 'Module Verifier' on function '@n'
Segmentation fault

Thanks @hans for the prompt revert!

hans added a comment.Mar 10 2023, 6:15 AM

In a different build configuration we hit

Value.cpp:504: void llvm::Value::doRAUW(Value *, ReplaceMetadataUses): Assertion `!contains(New, this) && "this->replaceAllUsesWith(expr(this)) is NOT valid!"' failed.

reproducer here: https://bugs.chromium.org/p/chromium/issues/detail?id=1423372#c1

FYI, "Invalid shuffle vector instruction operands!" assertion kicks in with both first and second commits: https://github.com/llvm/llvm-project/issues/61395

We're still seeing issues with this at trunk, even after the followup assertion fix in 874c49f55454cb285282e6d184f809945c0beca1.

One strange thing I noticed is an entire method that now gets replaced w/ unreachable. This may just be a bug in SimplifyCFGPass that this patch triggers: https://godbolt.org/z/1G6PTd6os

(That method isn't the one that's causing problems for us, but maybe it has the same root cause)

We're still seeing issues with this at trunk, even after the followup assertion fix in 874c49f55454cb285282e6d184f809945c0beca1.

One strange thing I noticed is an entire method that now gets replaced w/ unreachable. This may just be a bug in SimplifyCFGPass that this patch triggers: https://godbolt.org/z/1G6PTd6os

(That method isn't the one that's causing problems for us, but maybe it has the same root cause)

Thanks for the reproducer, will check it on Monday.

a more succinct version: https://godbolt.org/z/7YTqP89Mv shows that slp-vectorizer introduces loads from poison

a more succinct version: https://godbolt.org/z/7YTqP89Mv shows that slp-vectorizer introduces loads from poison

Already found the problem, going to commit a small fix soon.

a more succinct version: https://godbolt.org/z/7YTqP89Mv shows that slp-vectorizer introduces loads from poison

Must be fixed in 59ff9d3777701ebbe6a59ab2edb8792ef3d2873f

a more succinct version: https://godbolt.org/z/7YTqP89Mv shows that slp-vectorizer introduces loads from poison

Must be fixed in 59ff9d3777701ebbe6a59ab2edb8792ef3d2873f

I spot checked a few things and it all looks good so far, thanks!