This is an archive of the discontinued LLVM Phabricator instance.

Differential D61024

[SLP] Fix crash after r358519
ClosedPublic

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

Details

Reviewers
RKSimon
ABataev
dtemirbulatov
Commits
rGef3c1884ecd4: [SLP] Fix crash after r358519, by V. Porpodas.
rL359136: [SLP] Fix crash after r358519, by V. Porpodas.
Summary

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

Diff Detail

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
25

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

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

792

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

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.

792

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

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

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.

uabelho added a comment.Apr 24 2019, 1:00 AM

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

test/Transforms/SLPVectorizer/X86/crash_reordering_undefs.ll
2

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
4

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.

ABataev accepted this revision.Apr 24 2019, 12:22 PM

LG

This revision is now accepted and ready to land.Apr 24 2019, 12:22 PM
vporpo added a comment.Apr 24 2019, 12:40 PM

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

Closed by commit rL359136: [SLP] Fix crash after r358519, by V. Porpodas. (authored by ABataev). · Explain WhyApr 24 2019, 1:19 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: llvm-commits. · View Herald TranscriptApr 24 2019, 1:19 PM

Revision Contents

PathSize
lib/
Transforms/
Vectorize/
3 lines
test/
Transforms/
SLPVectorizer/
X86/
47 lines

Diff 196494

lib/Transforms/Vectorize/SLPVectorizer.cpp

Show First 20 Lines • Show All 766 Lines • ▼ Show 20 Lines getBestOperand(unsigned OpIdx, int Lane, int LastLane,
// different opcode in the linearized tree form. This would break the // different opcode in the linearized tree form. This would break the
// semantics. // semantics.
if (OpAPO != OpIdxAPO) if (OpAPO != OpIdxAPO)
continue; continue;
// Look for an operand that matches the current mode. // Look for an operand that matches the current mode.
switch (RMode) { switch (RMode) {
case ReorderingMode::Load: case ReorderingMode::Load:
if (isa<LoadInst>(Op)) { if (isa<LoadInst>(Op)) {
ABataevUnsubmitted
Not Done
ReplyInline Actions

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

ABataev: Probably, here you also may have a problem with `OpLastLane` as `UndefValue`, no?
vporpoAuthorUnsubmitted
Done
ReplyInline Actions

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.

vporpo: I don't think we are having this issue here, because ReorderingMode::Load only accepts loads.
// Figure out which is left and right, so that we can check for // Figure out which is left and right, so that we can check for
// consecutive loads // consecutive loads
bool LeftToRight = Lane > LastLane; bool LeftToRight = Lane > LastLane;
Value *OpLeft = (LeftToRight) ? OpLastLane : Op; Value *OpLeft = (LeftToRight) ? OpLastLane : Op;
Value *OpRight = (LeftToRight) ? Op : OpLastLane; Value *OpRight = (LeftToRight) ? Op : OpLastLane;
if (isConsecutiveAccess(cast<LoadInst>(OpLeft), if (isConsecutiveAccess(cast<LoadInst>(OpLeft),
cast<LoadInst>(OpRight), DL, SE)) cast<LoadInst>(OpRight), DL, SE))
BestOp.Idx = Idx; BestOp.Idx = Idx;
} }
break; break;
case ReorderingMode::Opcode: case ReorderingMode::Opcode:
// We accept both Instructions and Undefs, but with different scores. // We accept both Instructions and Undefs, but with different scores.
if ((isa<Instruction>(Op) && if ((isa<Instruction>(Op) && isa<Instruction>(OpLastLane) &&
cast<Instruction>(Op)->getOpcode() == cast<Instruction>(Op)->getOpcode() ==
cast<Instruction>(OpLastLane)->getOpcode()) || cast<Instruction>(OpLastLane)->getOpcode()) ||
(isa<UndefValue>(OpLastLane) && isa<Instruction>(Op)) ||
isa<UndefValue>(Op)) { isa<UndefValue>(Op)) {
ABataevUnsubmitted
Not Done
ReplyInline Actions

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

ABataev: What if `OpLastLane` is also `UndefValue`? Should this be accepted here too?
vporpoAuthorUnsubmitted
Done
ReplyInline Actions

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

vporpo: I guess we could accept that too. Let me try it out.
ABataevUnsubmitted
Not Done
ReplyInline Actions

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?

ABataev: If the `OpLastLane` is the instruction and `Op` is `Undef`, maybe `OpIdx` must be considered as…
vporpoAuthorUnsubmitted
Done
ReplyInline Actions

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.

vporpo: Yes, we will accept the Undef, if the other `Op` is not an Instruction. This is what the…
// An instruction has a higher score than an undef. // An instruction has a higher score than an undef.
unsigned Score = (isa<UndefValue>(Op)) ? GoodScore : BestScore; unsigned Score = (isa<UndefValue>(Op)) ? GoodScore : BestScore;
if (Score > BestOp.Score) { if (Score > BestOp.Score) {
BestOp.Idx = Idx; BestOp.Idx = Idx;
BestOp.Score = Score; BestOp.Score = Score;
} }
} }
break; break;
▲ Show 20 Lines • Show All 6,035 Lines • Show Last 20 Lines

test/Transforms/SLPVectorizer/X86/crash_reordering_undefs.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -slp-vectorizer -S < %s -mtriple=x86_64-unknown-linux -mcpu=corei7-avx | FileCheck %s
uabelhoUnsubmitted
Done
ReplyInline Actions

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.

uabelho: Often people prefer to have at least a few CHECKs that the output is reasonable in some way…
define i32 @crash_reordering_undefs() {
ABataevUnsubmitted
Done
ReplyInline Actions

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

ABataev: I think you can remove this from the test since the parameter with the triple is provided
; CHECK-LABEL: @crash_reordering_undefs(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[OR0:%.*]] = or i64 undef, undef
; CHECK-NEXT: [[CMP0:%.*]] = icmp eq i64 undef, [[OR0]]
; CHECK-NEXT: [[ADD0:%.*]] = select i1 [[CMP0]], i32 65536, i32 65537
; CHECK-NEXT: [[ADD1:%.*]] = add i32 undef, [[ADD0]]
; CHECK-NEXT: [[CMP1:%.*]] = icmp eq i64 undef, undef
; CHECK-NEXT: [[ADD2:%.*]] = select i1 [[CMP1]], i32 65536, i32 65537
; CHECK-NEXT: [[ADD3:%.*]] = add i32 [[ADD1]], [[ADD2]]
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i64 undef, undef
; CHECK-NEXT: [[ADD4:%.*]] = select i1 [[CMP2]], i32 65536, i32 65537
; CHECK-NEXT: [[ADD5:%.*]] = add i32 [[ADD3]], [[ADD4]]
; CHECK-NEXT: [[ADD6:%.*]] = add i32 [[ADD5]], undef
; CHECK-NEXT: [[ADD7:%.*]] = add i32 [[ADD6]], undef
; CHECK-NEXT: [[ADD8:%.*]] = add i32 [[ADD7]], undef
; CHECK-NEXT: [[OR1:%.*]] = or i64 undef, undef
; CHECK-NEXT: [[CMP3:%.*]] = icmp eq i64 undef, [[OR1]]
; CHECK-NEXT: [[ADD9:%.*]] = select i1 [[CMP3]], i32 65536, i32 65537
; CHECK-NEXT: [[ADD10:%.*]] = add i32 [[ADD8]], [[ADD9]]
; CHECK-NEXT: [[ADD11:%.*]] = add i32 [[ADD10]], undef
; CHECK-NEXT: ret i32 [[ADD11]]
RKSimonUnsubmitted
Done
ReplyInline Actions

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

RKSimon: Can you cleanup the test var names (-instnamer might do this for you)?
;
entry:
%or0 = or i64 undef, undef
%cmp0 = icmp eq i64 undef, %or0
%add0 = select i1 %cmp0, i32 65536, i32 65537
%add1 = add i32 undef, %add0
%cmp1 = icmp eq i64 undef, undef
%add2 = select i1 %cmp1, i32 65536, i32 65537
%add3 = add i32 %add1, %add2
%cmp2 = icmp eq i64 undef, undef
%add4 = select i1 %cmp2, i32 65536, i32 65537
%add5 = add i32 %add3, %add4
%add6 = add i32 %add5, undef
%add7 = add i32 %add6, undef
%add8 = add i32 %add7, undef
%or1 = or i64 undef, undef
%cmp3 = icmp eq i64 undef, %or1
%add9 = select i1 %cmp3, i32 65536, i32 65537
%add10 = add i32 %add8, %add9
%add11 = add i32 %add10, undef
ret i32 %add11
}