[IVDescriptor] Get the exact FP instruction that does not allow reordering

[congzhe]

Summary

This is a bugfix in IVDescriptor.cpp.

The helper function RecurrenceDescriptor::getExactFPMathInst() is supposed to return the 1st FP instruction that does not allow reordering. However, in certain cases it does not work as expected. For instance, in the test cases added in this patch, RecurrenceDescriptor::getExactFPMathInst() returns NULL for the reduction descriptor that corresponds to the reduction PHI node. This is because when constructing the RecurrenceDescriptor, we trace the use-def chain staring from a PHI node and for each instruction in the use-def chain, its descriptor overrides the previous one . Therefore in the final RecurrenceDescriptor we constructed., we lose previous FP instructions that does not allow reordering.

For the test case added in this patch, it should not be vectorized if reordering is not allowed. However with the current trunk it is vectorized.

[kmclaughlin]

Thank you for this fix @congzhe, LGTM! I have just added one minor comment on the new test.

llvm/test/Transforms/LoopVectorize/AArch64/strict-fadd.ll

593

Can you please add some CHECK-UNORDERED lines here? I believe for this test we should vectorise when reordering is allowed.

[congzhe]

Thanks for the review! I updated the patch accordingly.

llvm/lib/Analysis/IVDescriptors.cpp

bool RecurrenceDescriptor::AddReductionVar(PHINode *Phi, RecurKind Kind,
 
   Worklist.push_back(Start);
   VisitedInsts.insert(Start);
 
   // Start with all flags set because we will intersect this with the reduction
   // flags from all the reduction operations.
   FastMathFlags FMF = FastMathFlags::getFast();
 
+  // The first instruction in the use-def chain of the Phi node that requires
+  // exact floating point operations.
+  Instruction *ExactFPMathInst = nullptr;
+
   // A value in the reduction can be used:
   //  - By the reduction:
   //      - Reduction operation:
   //        - One use of reduction value (safe).
   //        - Multiple use of reduction value (not safe).
   //      - PHI:
   //        - All uses of the PHI must be the reduction (safe).
   //        - Otherwise, not safe.
if (!Cur->isCommutative() && !IsAPhi && !isa<SelectInst>(Cur) &&
       return false;
 
     // Any reduction instruction must be of one of the allowed kinds. We ignore
     // the starting value (the Phi or an AND instruction if the Phi has been
     // type-promoted).
     if (Cur != Start) {
       ReduxDesc =
           isRecurrenceInstr(TheLoop, Phi, Cur, Kind, ReduxDesc, FuncFMF);
+      ExactFPMathInst = ExactFPMathInst == nullptr
+                            ? ReduxDesc.getExactFPMathInst()
+                            : ExactFPMathInst;
       if (!ReduxDesc.isRecurrence())
         return false;
       // FIXME: FMF is allowed on phi, but propagation is not handled correctly.
       if (isa<FPMathOperator>(ReduxDesc.getPatternInst()) && !IsAPhi) {
         FastMathFlags CurFMF = ReduxDesc.getPatternInst()->getFastMathFlags();
         if (auto *Sel = dyn_cast<SelectInst>(ReduxDesc.getPatternInst())) {
           // Accept FMF on either fcmp or select of a min/max idiom.
           // TODO: This is a hack to work-around the fact that FMF may not be
if (isMinMaxRecurrenceKind(Kind) && NumCmpSelectPatternInst != 2 &&
     return false;
 
   if (isSelectCmpRecurrenceKind(Kind) && NumCmpSelectPatternInst != 1)
     return false;
 
   if (!FoundStartPHI || !FoundReduxOp || !ExitInstruction)
     return false;
 
-  const bool IsOrdered = checkOrderedReduction(
-      Kind, ReduxDesc.getExactFPMathInst(), ExitInstruction, Phi);
+  const bool IsOrdered =
+      checkOrderedReduction(Kind, ExactFPMathInst, ExitInstruction, Phi);
 
   if (Start != Phi) {
     // If the starting value is not the same as the phi node, we speculatively
     // looked through an 'and' instruction when evaluating a potential
     // arithmetic reduction to determine if it may have been type-promoted.
     //
     // We now compute the minimal bit width that is required to represent the
     // reduction. If this is the same width that was indicated by the 'and', we
bool RecurrenceDescriptor::AddReductionVar(PHINode *Phi, RecurKind Kind,
 
   // We found a reduction var if we have reached the original phi node and we
   // only have a single instruction with out-of-loop users.
 
   // The ExitInstruction(Instruction which is allowed to have out-of-loop users)
   // is saved as part of the RecurrenceDescriptor.
 
   // Save the description of this reduction variable.
-  RecurrenceDescriptor RD(RdxStart, ExitInstruction, Kind, FMF,
-                          ReduxDesc.getExactFPMathInst(), RecurrenceType,
-                          IsSigned, IsOrdered, CastInsts,
+  RecurrenceDescriptor RD(RdxStart, ExitInstruction, Kind, FMF, ExactFPMathInst,
+                          RecurrenceType, IsSigned, IsOrdered, CastInsts,
                           MinWidthCastToRecurrenceType);
   RedDes = RD;
 
   return true;
 }
 
 // We are looking for loops that do something like this:
 //   int r = 0;

llvm/test/Transforms/LoopVectorize/AArch64/strict-fadd.ll

for.body:                                         ; preds = %entry, %for.body
   %exitcond.not = icmp eq i64 %iv.next, %n
   br i1 %exitcond.not, label %for.end, label %for.body, !llvm.loop !0
 
 for.end:                                         ; preds = %for.body
   %rdx = phi float [ %add3, %for.body ]
   ret float %rdx
 }
 
+; Negative test - loop contains two fadds and only one fadd has the fast flag,
+; which we cannot safely reorder.
+define float @fadd_multiple_one_flag(float* noalias nocapture %a, float* noalias nocapture %b, i64 %n) {
+; CHECK-ORDERED-LABEL: @fadd_multiple_one_flag
+; CHECK-ORDERED-NOT: vector.body
+
+; CHECK-NOT-VECTORIZED-LABEL: @fadd_multiple_one_flag
+; CHECK-NOT-VECTORIZED-NOT: vector.body
+
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %entry, %for.body
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %sum = phi float [ -0.000000e+00, %entry ], [ %add3, %for.body ]
+  %arrayidx = getelementptr inbounds float, float* %a, i64 %iv
+  %0 = load float, float* %arrayidx, align 4
+  %add = fadd float %sum, %0
+  %arrayidx2 = getelementptr inbounds float, float* %b, i64 %iv
+  %1 = load float, float* %arrayidx2, align 4
+  %add3 = fadd fast float %add, %1
+  %iv.next = add nuw nsw i64 %iv, 1
+  %exitcond.not = icmp eq i64 %iv.next, %n
+  br i1 %exitcond.not, label %for.end, label %for.body, !llvm.loop !0
+
+for.end:                                         ; preds = %for.body
+  %rdx = phi float [ %add3, %for.body ]
+  ret float %rdx
+}
+
 ; Tests with both a floating point reduction & induction, e.g.
 ;
 ;float fp_iv_rdx_loop(float *values, float init, float * __restrict__ A, int N) {
 ;  float fp_inc = 2.0;
 ;  float x = init;
 ;  float sum = 0.0;
 ;  for (int i=0; i < N; ++i) {
 ;    A[i] = x;