[SLP] Fix crash after r358519

[vporpo]

Summary

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

[RKSimon]

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]

Cleaned up the lit test variable names.

[ABataev]

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]

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]

Accept undef in OpLastLane if Op contains an Instruction.

[ABataev]

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]

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.

[uabelho]

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]

Added FileCheck

[ABataev]

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]

Removed triple.

[ABataev]

LG

[vporpo]

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

llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp

getBestOperand(unsigned OpIdx, int Lane, int LastLane,
             Value *OpRight = (LeftToRight) ? Op : OpLastLane;
             if (isConsecutiveAccess(cast<LoadInst>(OpLeft),
                                     cast<LoadInst>(OpRight), DL, SE))
               BestOp.Idx = Idx;
           }
           break;
         case ReorderingMode::Opcode:
           // 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>(OpLastLane)->getOpcode()) ||
+              (isa<UndefValue>(OpLastLane) && isa<Instruction>(Op)) ||
               isa<UndefValue>(Op)) {
             // An instruction has a higher score than an undef.
             unsigned Score = (isa<UndefValue>(Op)) ? GoodScore : BestScore;
             if (Score > BestOp.Score) {
               BestOp.Idx = Idx;
               BestOp.Score = Score;
             }
           }

llvm/trunk/test/Transforms/SLPVectorizer/X86/crash_reordering_undefs.ll

+; 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
+
+define i32 @crash_reordering_undefs() {
+; 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]]
+;
+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
+}