[SLPVectorizer] Add propagateIRFlagsWithOp() function to propagate IRFlags for specific Operation

[dtemirbulatov]

Summary

This change is part of https://reviews.llvm.org/D28907.

This is a new functionality that allows to propagate IR flags only for specific operation OpValue parameter.

[RKSimon]

A few minor comments

lib/Transforms/Vectorize/SLPVectorizer.cpp

225

Put the Flag set: on the last line to make it clearer.

228

Should we assume this (the cast will assert on failure) or check for it?

if (auto *Intersection = dyn_cast<Instruction>(OpValue)) {
}

4585

Should the last argument be I[0]?

[filcab]

I haven't reviewed the usages of propagateIRFlagsWithOp, but this really needs test cases. Please add them.

lib/Transforms/Vectorize/SLPVectorizer.cpp

222

Does this need doxygen comments, or would a // be enough?
Please reformat the comment. The line widths aren't consistent.

227

Remove indentation levels like you do further in the function:

auto *VecOp = dyn_cast<Instruction>(I);
if (!VecOp)
  return;
// rest of the function

[filcab]

I don't get what you want to do here, sorry.
As I see it, in all the uses of propagateIRFlagsWithOp(X, Y, Z), we have Z == Y[0]. Which will end up being *almost* the same as the propagateIRFlags but with the side-effect that Z will also have those flags.
I'm not sure if Z having those flags is on purpose, though. And I don't see what we get from that.

Please tell me if I misunderstood something. I'm not the most familiar with the SLP vectorizer.

lib/Transforms/Vectorize/SLPVectorizer.cpp

226

Alternative name: propagateIRFlagsToOp since you're propagating them to that specific op. What do you think?

2858

What will be the difference between the old code and this one?

[dtemirbulatov]

lib/Transforms/Vectorize/SLPVectorizer.cpp

222

There was already doxygen comments before for this function, why to remove those?
oh, thanks, I will reformat.

225

ok

226

looks good

227

ok, correct

228

ok

2858

Here we propagate flags to V0 if only EvenScalars[0].getCode() equals to operations in V0 with similar Code, Also note how we gather flags in the new version and set those flags after that.

4585

correct.

[dtemirbulatov]

I don't get what you want to do here, sorry.
As I see it, in all the uses of propagateIRFlagsWithOp(X, Y, Z), we have Z == Y[0]. Which will end up being *almost* the same as the propagateIRFlags but with the side-effect that Z will also have those flags.

propagateIRFlagsWithOp only collects flags from Y if Y.Code == Z.Code while propagateIRFlags copies any flag from Y.

[RKSimon]

In D35292#806791, @dtemirbulatov wrote:

I don't get what you want to do here, sorry.
As I see it, in all the uses of propagateIRFlagsWithOp(X, Y, Z), we have Z == Y[0]. Which will end up being *almost* the same as the propagateIRFlags but with the side-effect that Z will also have those flags.

propagateIRFlagsWithOp only collects flags from Y if Y.Code == Z.Code while propagateIRFlags copies any flag from Y.

Please can you explain that difference from propagateIRFlags in the comment?

[dtemirbulatov]

I'm not sure if Z having those flags is on purpose, though. And I don't see what we get from that.

Please tell me if I misunderstood something. I'm not the most familiar with the SLP vectorizer.

Usually it should have flags in common and it collects those flags see "Intersection->andIRFlags(V);" in the loop across all VL,
but I have one example from test/Transforms/SLPVectorizer/X86/horizontal-list.ll where NUW flag was canceled by NSW.
VL[0]: %r1 = add nuw i32 %arg, undef
VL[1]: %r2 = add nsw i32 %r1, undef
VL[2]: %r3 = add nsw i32 %r2, undef
VL[3]: %r4 = add nsw i32 %r3, undef
VL[4]: %r5 = add nsw i32 %r4, undef

[dtemirbulatov]

Resulting in to:
VL[0]: %r1 = add i32 %arg, undef
VL[1]: %r2 = add nsw i32 %r1, undef
VL[2]: %r3 = add nsw i32 %r2, undef
VL[3]: %r4 = add nsw i32 %r3, undef
VL[4]: %r5 = add nsw i32 %r4, undef

[dtemirbulatov]

lib/Transforms/Vectorize/SLPVectorizer.cpp

227

ok

[dtemirbulatov]

Fixed issue with test/Transforms/SLPVectorizer/X86/horizontal-list.ll, Merged two versions of propagateIRFlags into one.

[RKSimon]

lib/Transforms/Utils/LoopUtils.cpp

1383 ↗

(On Diff #106497)

These must be dyn_cast and we must check for null.

1396 ↗

(On Diff #106497)

clang-format this whole function.

[dtemirbulatov]

update after rksimon's remarks.

[filcab]

LGTM

include/llvm/Transforms/Utils/LoopUtils.h

535 ↗

(On Diff #106611)

Maybe "If OpValue is non-null, we only consider operations similar to OpValue when intersecting"?

lib/Transforms/Utils/LoopUtils.cpp

1396 ↗

(On Diff #106611)

Merge these:

if (OpValue == nullptr || Opcode == Instr->getOpcode())
  VecOp->andIRFlags(V);

lib/Transforms/Vectorize/SLPVectorizer.cpp

if (!I || Opcode != I->getOpcode()) {
       if (canCombineAsAltInst(Opcode) && i == 1)
         return isAltInst(VL);
       return 0;
     }
   }
   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)
+/// only in case of scalar sub-operation from (VL) is equal to (OpValue). Flag set:
+/// NSW, NUW, exact, and all of fast-math.
+static void propagateIRFlagsWithOp(Value *I, ArrayRef<Value *> VL, Value *OpValue) {
+  if (auto *VecOp = dyn_cast<Instruction>(I)) {
+    auto *Intersection = cast<Instruction>(OpValue);
+    const unsigned Opcode = Intersection->getOpcode();
+    for (auto *V : VL) {
+      auto *Instr = dyn_cast<Instruction>(V);
+      if (!Instr)
+        continue;
+      if (Opcode == Instr->getOpcode())
+        Intersection->andIRFlags(V);
+    }
+    VecOp->copyIRFlags(Intersection);
+  }
+}
+
 /// \returns true if all of the values in \p VL have the same type or false
 /// otherwise.
 static bool allSameType(ArrayRef<Value *> VL) {
   Type *Ty = VL[0]->getType();
   for (int i = 1, e = VL.size(); i < e; i++)
     if (VL[i]->getType() != Ty)
       return 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);
+      propagateIRFlagsWithOp(E->VectorizedValue, E->Scalars, VL0);
       ++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));
case Instruction::Xor: {
       Value *RHS = vectorizeTree(RHSVL);
 
       if (Value *V = alreadyVectorized(E->Scalars))
         return V;
 
       BinaryOperator *BinOp = cast<BinaryOperator>(VL0);
       Value *V = Builder.CreateBinOp(BinOp->getOpcode(), LHS, RHS);
       E->VectorizedValue = V;
-      propagateIRFlags(E->VectorizedValue, E->Scalars);
+      propagateIRFlagsWithOp(E->VectorizedValue, E->Scalars, VL0);
       ++NumVectorInstructions;
 
       if (Instruction *I = dyn_cast<Instruction>(V))
         return propagateMetadata(I, E->Scalars);
 
       return V;
     }
     case Instruction::Load: {
case Instruction::Call: {
 
       // The scalar argument uses an in-tree scalar so we add the new vectorized
       // call to ExternalUses list to make sure that an extract will be
       // generated in the future.
       if (ScalarArg && getTreeEntry(ScalarArg))
         ExternalUses.push_back(ExternalUser(ScalarArg, cast<User>(V), 0));
 
       E->VectorizedValue = V;
-      propagateIRFlags(E->VectorizedValue, E->Scalars);
+      propagateIRFlagsWithOp(E->VectorizedValue, E->Scalars, VL0);
       ++NumVectorInstructions;
       return V;
     }
     case Instruction::ShuffleVector: {
       ValueList LHSVL, RHSVL;
       assert(isa<BinaryOperator>(VL0) && "Invalid Shuffle Vector Operand");
       reorderAltShuffleOperands(E->Scalars, LHSVL, RHSVL);
       setInsertPointAfterBundle(E->Scalars);
case Instruction::ShuffleVector: {
           OddScalars.push_back(E->Scalars[i]);
         } else {
           Mask[i] = Builder.getInt32(i);
           EvenScalars.push_back(E->Scalars[i]);
         }
       }
 
       Value *ShuffleMask = ConstantVector::get(Mask);
-      propagateIRFlags(V0, EvenScalars);
-      propagateIRFlags(V1, OddScalars);
+      propagateIRFlagsWithOp(V0, EvenScalars, EvenScalars[0]);
+      propagateIRFlagsWithOp(V1, OddScalars, OddScalars[0]);
 
       Value *V = Builder.CreateShuffleVector(V0, V1, ShuffleMask);
       E->VectorizedValue = V;
       ++NumVectorInstructions;
       if (Instruction *I = dyn_cast<Instruction>(V))
         return propagateMetadata(I, E->Scalars);
 
       return V;
while (i < NumReducedVals - ReduxWidth + 1 && ReduxWidth > 2) {
 
       // Emit a reduction.
       Value *ReducedSubTree =
           emitReduction(VectorizedRoot, Builder, ReduxWidth, ReductionOps, TTI);
       if (VectorizedTree) {
         Builder.SetCurrentDebugLocation(Loc);
         VectorizedTree = Builder.CreateBinOp(ReductionOpcode, VectorizedTree,
                                              ReducedSubTree, "bin.rdx");
-        propagateIRFlags(VectorizedTree, ReductionOps);
+        propagateIRFlagsWithOp(VectorizedTree, ReductionOps, ReductionOps[0]);
       } else
         VectorizedTree = ReducedSubTree;
       i += ReduxWidth;
       ReduxWidth = PowerOf2Floor(NumReducedVals - i);
     }
 
     if (VectorizedTree) {
       // Finish the reduction.
       for (; i < NumReducedVals; ++i) {
         auto *I = cast<Instruction>(ReducedVals[i]);
         Builder.SetCurrentDebugLocation(I->getDebugLoc());
         VectorizedTree =
             Builder.CreateBinOp(ReductionOpcode, VectorizedTree, I);
-        propagateIRFlags(VectorizedTree, ReductionOps);
+        propagateIRFlagsWithOp(VectorizedTree, ReductionOps, ReductionOps[0]);
       }
       for (auto &Pair : ExternallyUsedValues) {
         assert(!Pair.second.empty() &&
                "At least one DebugLoc must be inserted");
         // Add each externally used value to the final reduction.
         for (auto *I : Pair.second) {
           Builder.SetCurrentDebugLocation(I->getDebugLoc());
           VectorizedTree = Builder.CreateBinOp(ReductionOpcode, VectorizedTree,
                                                Pair.first, "bin.extra");
-          propagateIRFlags(VectorizedTree, I);
+          propagateIRFlagsWithOp(VectorizedTree, I, I);
         }
       }
       // Update users.
       ReductionRoot->replaceAllUsesWith(VectorizedTree);
     }
     return VectorizedTree != nullptr;
   }
 
for (unsigned i = ReduxWidth / 2; i != 0; i >>= 1) {
 
       Value *LeftShuf = Builder.CreateShuffleVector(
           TmpVec, UndefValue::get(TmpVec->getType()), LeftMask, "rdx.shuf.l");
       Value *RightShuf = Builder.CreateShuffleVector(
           TmpVec, UndefValue::get(TmpVec->getType()), (RightMask),
           "rdx.shuf.r");
       TmpVec =
           Builder.CreateBinOp(ReductionOpcode, LeftShuf, RightShuf, "bin.rdx");
-      propagateIRFlags(TmpVec, RedOps);
+      propagateIRFlagsWithOp(TmpVec, RedOps, RedOps[0]);
     }
 
     // The result is in the first element of the vector.
     return Builder.CreateExtractElement(TmpVec, Builder.getInt32(0));
   }
 };
 } // end anonymous namespace