Fix for lost FastMathFlags in SLPVectorizer

[v_klochkov]

Summary

Hello,

Please review the fix for SLPVectorizer that lost FastMathFlags in FCmp operation.

Initially this fix was proposed together with few other similar fixes
(https://reviews.llvm.org/D26436),
but since then the fix has been re-worked and some reviewers recommendations to have
separate code-reviews for each of the fixed components were taken into account.

Some explanations for this fix:

1. The call of the method propagateIRFlags() is performed not only for FCmp. It is called for ICmp as well. It is done the same way as it is done for binary operations (i.e. the method is called even for int ADD). I think this is right approach and the method propagateIRFlags() should decide what flags and for what operations it propagates.

2. The fix replaces the casts to <BinaryOperator> with casts to <Instruction> inside the method propagateIRFlags(). The whole work done in this method is done with help of Instruction::andIRFlags(). In my opinion, the AND'able flags used in the set of original/scalar operations should all be AND'ed. Please fix me if this is wrong or too risky.
   
   The alternative was to have duplicated code due to not quite clear reasons, i.e.: if (auto *VecOp = dyn_cast<BinaryOperator>(I)) { <9 lines of code here> } else if (auto *VecOp = dyn_cast<CmpInst>(I)) { <exactly the same/similar 9 lines of code here> }

The fix includes 2 new test cases inside an existing LIT test.
Also, all other LIT tests successfully passed.

Thank you,
Vyacheslav Klochkov

[mzolotukhin]

LGTM! I wonder if there are other types of instructions that can have flags (except BinaryOperation and CmpInstruction), which we need to check as well.

Michael

llvm/test/Transforms/SLPVectorizer/X86/propagate_ir_flags.ll

351

Can we put that in the bottom of the file?

[v_klochkov]

`Thank you Michael,

I am not sure about what SLPVectorizer does for calls..
If it can replace a series of scalar calls (probably intrinsic calls) with 1 call returning FP-vector,
then it may want to propagate FAST flags.

For example I can see fast flags in such LLVM IR:

define double @_Z3foov() #0 {
entry:
  %call = call fast double @_Z3barv()
  ret double %call
}

`

llvm/test/Transforms/SLPVectorizer/X86/propagate_ir_flags.ll

351

Ok, I moved it to the end of the file.

[mzolotukhin]

Hi Vyacheslav,

Yes, SLP vectorizer does work with calls (see test/Transforms/SLPVectorizer/X86/intrinsic.ll), and I guess we're currently dropping flags there as well. Do you mind putting a TODO there/fixing it/filing a bug?

Thanks,
Michael

[v_klochkov]

`Ok, done. Similar fix for intrinsic calls is submitted here: https://reviews.llvm.org/D26575

Thank you,
Vyacheslav Klochkov
`

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

static unsigned getSameOpcode(ArrayRef<Value *> VL) {
   }
   return Opcode;
 }
 
 /// Get the intersection (logical and) of all of the potential IR flags
 /// of each scalar operation (VL) that will be converted into a vector (I).
 /// Flag set: NSW, NUW, exact, and all of fast-math.
 static void propagateIRFlags(Value *I, ArrayRef<Value *> VL) {
-  if (auto *VecOp = dyn_cast<BinaryOperator>(I)) {
-    if (auto *Intersection = dyn_cast<BinaryOperator>(VL[0])) {
+  if (auto *VecOp = dyn_cast<Instruction>(I)) {
+    if (auto *Intersection = dyn_cast<Instruction>(VL[0])) {
       // Intersection is initialized to the 0th scalar,
       // so start counting from index '1'.
       for (int i = 1, e = VL.size(); i < e; ++i) {
-        if (auto *Scalar = dyn_cast<BinaryOperator>(VL[i]))
+        if (auto *Scalar = dyn_cast<Instruction>(VL[i]))
           Intersection->andIRFlags(Scalar);
       }
       VecOp->copyIRFlags(Intersection);
     }
   }
 }
 
 /// \returns true if all of the values in \p VL have the same type or false
case Instruction::ICmp: {
       CmpInst::Predicate P0 = cast<CmpInst>(VL0)->getPredicate();
       Value *V;
       if (Opcode == Instruction::FCmp)
         V = Builder.CreateFCmp(P0, L, R);
       else
         V = Builder.CreateICmp(P0, L, R);
 
       E->VectorizedValue = V;
+      propagateIRFlags(E->VectorizedValue, E->Scalars);
       ++NumVectorInstructions;
       return V;
     }
     case Instruction::Select: {
       ValueList TrueVec, FalseVec, CondVec;
       for (Value *V : E->Scalars) {
         CondVec.push_back(cast<Instruction>(V)->getOperand(0));
         TrueVec.push_back(cast<Instruction>(V)->getOperand(1));

llvm/test/Transforms/SLPVectorizer/X86/propagate_ir_flags.ll

define void @addsub_no_nsw(i32* %x) {
   store i32 %op1, i32* %idx1, align 4
   store i32 %op2, i32* %idx2, align 4
   store i32 %op3, i32* %idx3, align 4
   store i32 %op4, i32* %idx4, align 4
 
   ret void
 }
  
+attributes #1 = { "target-features"="+avx" }
+
+; CHECK-LABEL: @fcmp_fast
+; CHECK: fcmp fast oge <2 x double>
+; CHECK: sub fast <2 x double>
+define void @fcmp_fast(double* %x) #1 {
+  %idx1 = getelementptr inbounds double, double* %x, i64 0
+  %idx2 = getelementptr inbounds double, double* %x, i64 1
+
+  %load1 = load double, double* %idx1, align 8
+  %load2 = load double, double* %idx2, align 8
+
+  %cmp1 = fcmp fast oge double %load1, 0.000000e+00
+  %cmp2 = fcmp fast oge double %load2, 0.000000e+00
+
+  %sub1 = fsub fast double -0.000000e+00, %load1
+  %sub2 = fsub fast double -0.000000e+00, %load2
+
+  %sel1 = select i1 %cmp1, double %load1, double %sub1
+  %sel2 = select i1 %cmp2, double %load2, double %sub2
+
+  store double %sel1, double* %idx1, align 8
+  store double %sel2, double* %idx2, align 8
+
+  ret void
+}
+
+; CHECK-LABEL: @fcmp_no_fast
+; CHECK: fcmp oge <2 x double>
+; CHECK: sub <2 x double>
+define void @fcmp_no_fast(double* %x) #1 {
+  %idx1 = getelementptr inbounds double, double* %x, i64 0
+  %idx2 = getelementptr inbounds double, double* %x, i64 1
+
+  %load1 = load double, double* %idx1, align 8
+  %load2 = load double, double* %idx2, align 8
+
+  %cmp1 = fcmp fast oge double %load1, 0.000000e+00
+  %cmp2 = fcmp oge double %load2, 0.000000e+00
+
+  %sub1 = fsub fast double -0.000000e+00, %load1
+  %sub2 = fsub double -0.000000e+00, %load2
+
+  %sel1 = select i1 %cmp1, double %load1, double %sub1
+  %sel2 = select i1 %cmp2, double %load2, double %sub2
+
+  store double %sel1, double* %idx1, align 8
+  store double %sel2, double* %idx2, align 8
+
+  ret void
+}