[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 ↗

(On Diff #106140)

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

228 ↗

(On Diff #106140)

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

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

4585 ↗

(On Diff #106140)

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 ↗

(On Diff #106140)

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

227 ↗

(On Diff #106140)

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 ↗

(On Diff #106140)

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

2858 ↗

(On Diff #106140)

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

[dtemirbulatov]

lib/Transforms/Vectorize/SLPVectorizer.cpp

222 ↗

(On Diff #106140)

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

225 ↗

(On Diff #106140)

ok

226 ↗

(On Diff #106140)

looks good

227 ↗

(On Diff #106140)

ok, correct

228 ↗

(On Diff #106140)

ok

2858 ↗

(On Diff #106140)

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 ↗

(On Diff #106140)

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 ↗

(On Diff #106140)

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

These must be dyn_cast and we must check for null.

1396

clang-format this whole function.

[dtemirbulatov]

update after rksimon's remarks.

[filcab]

LGTM

include/llvm/Transforms/Utils/LoopUtils.h

535

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

lib/Transforms/Utils/LoopUtils.cpp

1396

Merge these:

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

include/llvm/Transforms/Utils/LoopUtils.h

 /// The target is queried to determine if intrinsics or shuffle sequences are
 /// required to implement the reduction.
 Value *createTargetReduction(IRBuilder<> &B, const TargetTransformInfo *TTI,
                              RecurrenceDescriptor &Desc, Value *Src,
                              bool NoNaN = false);
 
 /// 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).
+/// If OpValue is set then we consider only similar operation to OpValue in
+/// the intersection.
 /// Flag set: NSW, NUW, exact, and all of fast-math.
-void propagateIRFlags(Value *I, ArrayRef<Value *> VL);
+void propagateIRFlags(Value *I, ArrayRef<Value *> VL, Value *OpValue = nullptr);
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_UTILS_LOOPUTILS_H

lib/Transforms/Utils/LoopUtils.cpp

Flags.IsMaxOp =
         Desc.getMinMaxRecurrenceKind() == RecurrenceDescriptor::MRK_FloatMax;
     return getSimpleRdx(Instruction::FCmp);
   }
   default:
     llvm_unreachable("Unhandled RecKind");
   }
 }
 
-void llvm::propagateIRFlags(Value *I, ArrayRef<Value *> VL) {
-  if (auto *VecOp = dyn_cast<Instruction>(I)) {
-    if (auto *I0 = dyn_cast<Instruction>(VL[0])) {
-      // VecOVp is initialized to the 0th scalar, so start counting from index
-      // '1'.
-      VecOp->copyIRFlags(I0);
-      for (int i = 1, e = VL.size(); i < e; ++i) {
-        if (auto *Scalar = dyn_cast<Instruction>(VL[i]))
-          VecOp->andIRFlags(Scalar);
-      }
-    }
+void llvm::propagateIRFlags(Value *I, ArrayRef<Value *> VL, Value *OpValue) {
+  auto *VecOp = dyn_cast<Instruction>(I);
+  if (!VecOp) return;
+  auto *Intersection = (OpValue == nullptr) ? cast<Instruction>(VL[0]) :
+                                      cast<Instruction>(OpValue);
+  const unsigned Opcode = Intersection->getOpcode();
+  VecOp->copyIRFlags(Intersection);
+  for (auto *V : VL) {
+    auto *Instr = dyn_cast<Instruction>(V);
+    if (!Instr)
+       continue;
+    if (OpValue == nullptr)
+            VecOp->andIRFlags(V);
+      else
+    if (Opcode == Instr->getOpcode())
+                       VecOp->andIRFlags(V);
   }
 }