[IVDescriptors] Fix bug in checkOrderedReduction

[anna]

Summary

The Exit instruction passed in for checking if it's an ordered reduction need not be
an FPAdd operation. We need to bail out at that point instead of
assuming it is an FPAdd (and hence has two operands). See added testcase.
It crashes without the patch because the Exit instruction is a phi with
exactly one operand.
This latent bug was exposed by 95346ba which added support for
multi-exit loops for vectorization.

[kmclaughlin]

Thank you for this fix, @anna - LGTM

llvm/test/Transforms/LoopVectorize/fp-reduction-crash.ll

2

nit: I don't think REQUIRES: asserts is necessary for this test?

[anna]

llvm/test/Transforms/LoopVectorize/fp-reduction-crash.ll

2

This is to show that the failure only occurs in assertions enabled mode. It will not fail in any other mode.

To put it differently, if I first landed this testcase alone, it will succeed in non-assertion enabled mode and it isn't clear what the problem is (since we cannot add XFAIL).

[fhahn]

llvm/test/Transforms/LoopVectorize/fp-reduction-crash.ll

2

Can you add check lines to make sure the IR is transformed as expected? That way, we do not need to rely on the asserting causing the test to crash.

[fhahn]

llvm/test/Transforms/LoopVectorize/fp-reduction-crash.ll

2

actually, can the test be added to the file already containing order reduction tests, with a comment explaining what it checks? That will help make maintaining it easier.

[anna]

llvm/test/Transforms/LoopVectorize/fp-reduction-crash.ll

2

The reason it doesn't cause any failure in non-assertion mode is that this just checks and bails out without doing vectorization. The bail out was too late. That was the only reason this crash occurred.
So, the IR is not transformed for this reduction pattern - not sure what the CHECK-LINES will do. I will add a comment explaining what it checks and move it to the order-reduction tests.

[fhahn]

llvm/test/Transforms/LoopVectorize/fp-reduction-crash.ll

2

So, the IR is not transformed for this reduction pattern - not sure what the CHECK-LINES will do. I will add a comment explaining what it checks and move it to the order-reduction tests.

Sounds good to me, thanks!

llvm/lib/Analysis/IVDescriptors.cpp

 // Check if a given Phi node can be recognized as an ordered reduction for
 // vectorizing floating point operations without unsafe math.
 static bool checkOrderedReduction(RecurKind Kind, Instruction *ExactFPMathInst,
                                   Instruction *Exit, PHINode *Phi) {
   // Currently only FAdd is supported
   if (Kind != RecurKind::FAdd)
     return false;
 
-  bool IsOrdered =
-      Exit->getOpcode() == Instruction::FAdd && Exit == ExactFPMathInst;
+  if (Exit->getOpcode() != Instruction::FAdd || Exit != ExactFPMathInst)
+    return false;
 
   // The only pattern accepted is the one in which the reduction PHI
   // is used as one of the operands of the exit instruction
   auto *LHS = Exit->getOperand(0);
   auto *RHS = Exit->getOperand(1);
-  IsOrdered &= ((LHS == Phi) || (RHS == Phi));
-
-  if (!IsOrdered)
+  if (LHS != Phi && RHS != Phi)
     return false;
 
   LLVM_DEBUG(dbgs() << "LV: Found an ordered reduction: Phi: " << *Phi
                     << ", ExitInst: " << *Exit << "\n");
 
   return true;
 }
 

llvm/test/Transforms/LoopVectorize/fp-reduction-crash.ll

+; REQUIRES: asserts
+; RUN: opt < %s -loop-vectorize -S | FileCheck %s
+
+; CHECK-LABEL: quux
+define void @quux() {
+bb:
+  br label %bb4
+
+bb1:                                              ; preds = %bb4
+  %tmp = phi double [ %tmp6, %bb4 ]
+  br i1 undef, label %bb4, label %bb2
+
+bb2:                                              ; preds = %bb1
+  %tmp3 = phi double [ %tmp, %bb1 ]
+  ret void
+
+bb4:                                              ; preds = %bb1, %bb
+  %tmp5 = phi double [ 1.300000e+01, %bb ], [ %tmp, %bb1 ]
+  %tmp6 = fadd double %tmp5, 1.000000e+00
+  br label %bb1
+}