Page MenuHomePhabricator

[SLP] Fix crash in reduction for integer min/max
ClosedPublic

Authored by yrouban on Mar 25 2021, 4:35 AM.

Details

Summary

The SCEV change https://reviews.llvm.org/rGb46c085d2b6d15873fb53718f0a70b3848e19e4a seems to reveal a new crash in SLPVectorizer.
SLP crashes expecting a SelectInst as an externally used value but umin call is found.

The patch relaxes the assumption to make the IR flag propagation safe.

Diff Detail

Event Timeline

yrouban created this revision.Mar 25 2021, 4:35 AM
yrouban requested review of this revision.Mar 25 2021, 4:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2021, 4:35 AM

Looks good to me in general.

fhahn added a subscriber: fhahn.Mar 25 2021, 4:54 AM
fhahn added inline comments.
llvm/test/Transforms/SLPVectorizer/slp-umax-rdx-crash.ll
3 ↗(On Diff #333255)

There's no need to require asserts I think, given that you just check the IR. You also don't need 2 > &1 I think

yrouban marked an inline comment as done.Mar 25 2021, 5:04 AM
yrouban added inline comments.Mar 25 2021, 5:09 AM
llvm/test/Transforms/SLPVectorizer/slp-umax-rdx-crash.ll
12–25 ↗(On Diff #333255)

the check are wrong - fixed

yrouban updated this revision to Diff 333264.Mar 25 2021, 5:10 AM

fixed the test

spatel accepted this revision.Mar 25 2021, 6:08 AM

Code change LGTM.

Please just add this test to the file that we created for the similar problem in D98432. Note that we don't need asserts or redirect of stderr in that test file either.

This revision is now accepted and ready to land.Mar 25 2021, 6:08 AM
This revision was landed with ongoing or failed builds.Mar 25 2021, 8:03 AM
This revision was automatically updated to reflect the committed changes.