Page MenuHomePhabricator

[SLP] Fix crash after r358519
ClosedPublic

Authored by vporpo on Apr 23 2019, 8:59 AM.

Details

Summary

The code did not check if operand was undef before casting it to Instruction.

Diff Detail

Repository
rL LLVM

Event Timeline

vporpo created this revision.Apr 23 2019, 8:59 AM
RKSimon added inline comments.Apr 23 2019, 9:51 AM
test/Transforms/SLPVectorizer/X86/crash_reordering_undefs.ll
24 ↗(On Diff #196260)

Can you cleanup the test var names (-instnamer might do this for you)?

vporpo updated this revision to Diff 196275.Apr 23 2019, 10:16 AM

Cleaned up the lit test variable names.

ABataev added inline comments.Apr 23 2019, 10:18 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
775 ↗(On Diff #196260)

Probably, here you also may have a problem with OpLastLane as UndefValue, no?

791 ↗(On Diff #196260)

What if OpLastLane is also UndefValue? Should this be accepted here too?

vporpo marked 3 inline comments as done.Apr 23 2019, 10:31 AM
vporpo added inline comments.
lib/Transforms/Vectorize/SLPVectorizer.cpp
775 ↗(On Diff #196260)

I don't think we are having this issue here, because ReorderingMode::Load only accepts loads. If we run into an Undef while in ReorderingMode::Load, getBestOperand() will return none, and the mode will change to RerorderingMode::Failed in line 1013.

On the other hand ReorderingMode::Opcode accepts both Instructions and undefs.

791 ↗(On Diff #196260)

I guess we could accept that too. Let me try it out.

vporpo updated this revision to Diff 196299.Apr 23 2019, 12:20 PM

Accept undef in OpLastLane if Op contains an Instruction.

ABataev added inline comments.Apr 23 2019, 1:31 PM
lib/Transforms/Vectorize/SLPVectorizer.cpp
792 ↗(On Diff #196299)

If the OpLastLane is the instruction and Op is Undef, maybe OpIdx must be considered as BestOp.Idx? Or this handled already by good|best scores?

vporpo marked an inline comment as done.Apr 23 2019, 1:58 PM
vporpo added inline comments.
lib/Transforms/Vectorize/SLPVectorizer.cpp
792 ↗(On Diff #196299)

Yes, we will accept the Undef, if the other Op is not an Instruction. This is what the good/best scores are used for: they give higher priority to VLs with instructions only, as opposed to those with both instructions and undefs. So if let's say OpLastLane was an instruction and we have two candidate Ops, one Undef and one Instruction, we will choose the instruction, because it will get the BestScore.

In a similar situation, if we have to choose between two Ops, one instruction and one undef, but OpLastLane is an undef, then I am not sure which one would be better.

Thanks for fixing!
The original case where I saw the crash compiles successfully with this patch.

test/Transforms/SLPVectorizer/X86/crash_reordering_undefs.ll
1 ↗(On Diff #196299)

Often people prefer to have at least a few CHECKs that the output is reasonable in some way instead of just seeing that we don't get a crash.

vporpo updated this revision to Diff 196493.Apr 24 2019, 12:09 PM

Added FileCheck

vporpo marked an inline comment as done.Apr 24 2019, 12:09 PM
ABataev added inline comments.Apr 24 2019, 12:12 PM
test/Transforms/SLPVectorizer/X86/crash_reordering_undefs.ll
3 ↗(On Diff #196493)

I think you can remove this from the test since the parameter with the triple is provided

vporpo updated this revision to Diff 196494.Apr 24 2019, 12:17 PM
vporpo marked an inline comment as done.

Removed triple.

This revision is now accepted and ready to land.Apr 24 2019, 12:22 PM

Thank you for the reviews, and @uabelho for reporting the issue.
Please commit the patch as I don't have commit access.

This revision was automatically updated to reflect the committed changes.