[LoopVectorize] Fix assertion failure in Fcmp vectorization

[anna]

Summary

When vectorizing fcmps we can trip on incorrect cast assertion when setting the
FastMathFlags after generating the vectorized FCmp.
This can happen if the FCmp can be folded to true or false directly. The fix
here is to set the FastMathFlag using the FastMathFlagBuilder *before* creating
the FCmp Instruction. This is what's done by other optimizations such as
InstCombine.
Added a test case which trips on cast assertion without this patch.

[Ayal]

lib/Transforms/Vectorize/LoopVectorize.cpp

4867 ↗

(On Diff #109436)

An alternative may be to do

if (isa<FCmpInst>(C))
  cast<FCmpInst>(C)->copyFastMathFlags(Cmp);

or

if (auto *FCmp = dyn_cast<FCmpInst>(C))
  FCmp->copyFastMathFlags(Cmp);

Wonder if the last, optional argument to CreateFCmp() may be used instead?

Is this issue relevant elsewhere, e.g., SLP vectorizer? (Maybe that last argument shouldn't be optional ;-)

[anna]

lib/Transforms/Vectorize/LoopVectorize.cpp

4867 ↗

(On Diff #109436)

the alternative way may also be used.

The FastMathFlagGuard seems to be the accepted practice though - in all of instcombine and LoopUtils transformations where the created instruction may not be a compare.

SLPVectorizer creates FCmp and uses propagateIRFlags from LoopUtils for this. So, SLPVectorizer does not have this bug. However, it's a full blown method that copies IR flags by checking the type of instruction, and we don't need it here in LoopVectorizer.

[mssimpso]

Hi Anna,

Thanks for taking a look at this. I had a few comments about the test. Otherwise, it makes sense to me.

test/Transforms/LoopVectorize/fcmp-vectorize.ll

1 ↗

(On Diff #109436)

Since this is a target-independent test, it's probably a good idea to specify the vector width and interleave factor: -force-vector-width=4 -force-vector-interleave=1.

3 ↗

(On Diff #109436)

The test doesn't require assertions, so you can remove this line.

14–16 ↗

(On Diff #109436)

No need to check the original loop in this case.

[Ayal]

lib/Transforms/Vectorize/LoopVectorize.cpp

4867 ↗

(On Diff #109436)

this alternative way also appears in instcombine, in the form of:

auto *FPInst = dyn_cast<Instruction>(RI);
if (FPInst && isa<FPMathOperator>(FPInst))
  FPInst->copyFastMathFlags(BO);

and is also used by SLPVectorizer eventually via copyIRFlags()/andIRFlags().

If you prefer the FastMathFlagGuard way, suggest to also attach a preceding comment, e.g.:

// Propagate fast math flags.

BTW, curious how such a redundant comparison reached the vectorizer? Such things are better cleaned up earlier, to let the vectorizer and its cost model concentrate on 'real' instructions.

[anna]

Addressed review comments.

lib/Transforms/Vectorize/LoopVectorize.cpp

4867 ↗

(On Diff #109436)

I've added the comment. Would be good to find out how the redundant comparison came in, will take a look.

I think that's an orthogonal performance problem, because there can always be other cases where we have such redundant comparisons flowing through the vectorizer. For example, it could also be a missed optimization or an in-tree/out-of-tree pass ordering issue.

[Ayal]

Looks good to me, please wait for @mssimpso to approve.

[mssimpso]

LGTM as well.

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

case Instruction::FCmp: {
     bool FCmp = (I.getOpcode() == Instruction::FCmp);
     auto *Cmp = dyn_cast<CmpInst>(&I);
     setDebugLocFromInst(Builder, Cmp);
     for (unsigned Part = 0; Part < UF; ++Part) {
       Value *A = getOrCreateVectorValue(Cmp->getOperand(0), Part);
       Value *B = getOrCreateVectorValue(Cmp->getOperand(1), Part);
       Value *C = nullptr;
       if (FCmp) {
+        // Propagate fast math flags.
+        IRBuilder<>::FastMathFlagGuard FMFG(Builder);
+        Builder.setFastMathFlags(Cmp->getFastMathFlags());
         C = Builder.CreateFCmp(Cmp->getPredicate(), A, B);
-        cast<FCmpInst>(C)->copyFastMathFlags(Cmp);
       } else {
         C = Builder.CreateICmp(Cmp->getPredicate(), A, B);
       }
       VectorLoopValueMap.setVectorValue(&I, Part, C);
       addMetadata(C, &I);
     }
 
     break;

llvm/trunk/test/Transforms/LoopVectorize/fcmp-vectorize.ll

+; RUN: opt -loop-vectorize -force-vector-width=4 -force-vector-interleave=1 -S %s | FileCheck %s
+
+; Avoid crashing while trying to vectorize fcmp that can be folded to vector of
+; i1 true.
+define void @test1() {
+; CHECK-LABEL: test1(
+; CHECK-LABEL: vector.body:
+; CHECK-NEXT:    %index = phi i32 [ 0, %vector.ph ], [ %index.next, %vector.body ]
+; CHECK-NEXT:    %broadcast.splatinsert = insertelement <4 x i32> undef, i32 %index, i32 0
+; CHECK:         %induction = add <4 x i32> %broadcast.splat, <i32 0, i32 1, i32 2, i32 3>
+; CHECK:         %index.next = add i32 %index, 4
+
+entry:
+  br label %loop
+
+loop:                                              ; preds = %loop, %entry
+  %iv = phi i32 [ 0, %entry ], [ %ivnext, %loop ]
+  %fcmp = fcmp uno float 0.000000e+00, 0.000000e+00
+  %ivnext = add nsw i32 %iv, 1
+  %cnd = icmp sgt i32 %iv, 142
+  br i1 %cnd, label %exit, label %loop
+
+exit:                                              ; preds = %loop
+  ret void
+}