This is an archive of the discontinued LLVM Phabricator instance.

[SLP]Fix a crash in gathered loads analysis.
ClosedPublic

Authored by ABataev on Jul 29 2021, 5:07 AM.

Details

Summary

Need to check that the minimum acceptable vector factor is at least 2,
not 0, to avoid compiler crash during gathered loads analysis.

Diff Detail

Event Timeline

ABataev created this revision.Jul 29 2021, 5:07 AM
ABataev requested review of this revision.Jul 29 2021, 5:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2021, 5:07 AM
This revision is now accepted and ready to land.Jul 29 2021, 5:30 AM
spatel added inline comments.Jul 29 2021, 5:41 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7454

Similar logic...

7565

And again...can we consolidate these into a helper call to avoid the problem consistently?

Thanks for fixing!

llvm/test/Transforms/SLPVectorizer/AArch64/loads-gather-crash.ll
4 ↗(On Diff #362724)

Could you name the test to something more descriptive, e.g. gather-load-min-required-vf-2.ll (feel free to come up with something better) and also add a comment explaining why the code should not be slp-vectorized?

ABataev added inline comments.Jul 29 2021, 5:43 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7454

Will try.

llvm/test/Transforms/SLPVectorizer/AArch64/loads-gather-crash.ll
4 ↗(On Diff #362724)

Ok, no problem.

ABataev updated this revision to Diff 362751.Jul 29 2021, 6:21 AM

Address comments.

spatel accepted this revision.Jul 29 2021, 3:59 PM

Code changes LGTM. No test changes in the updated revision though?
Note that there's a new bugzilla report that has another potential test example for this bug:
https://llvm.org/PR51275

Code changes LGTM. No test changes in the updated revision though?
Note that there's a new bugzilla report that has another potential test example for this bug:
https://llvm.org/PR51275

No test changes and actually there should not be any. It is a corner case.

Code changes LGTM. No test changes in the updated revision though?
Note that there's a new bugzilla report that has another potential test example for this bug:
https://llvm.org/PR51275

No test changes and actually there should not be any. It is a corner case.

Ok - I saw the earlier request about adding a comment, and the file name changed, so maybe that’s enough. I didn’t try to reduce the example from bugzilla, but that shows it is possible to induce from legitimate C source code.

This revision was automatically updated to reflect the committed changes.