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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:222
 
+/// 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)
----------------
filcab wrote:
> Does this need doxygen comments, or would a `//` be enough?
> Please reformat the comment. The line widths aren't consistent.
There was already doxygen comments before for this function, why to remove those?
oh, thanks, I will reformat.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:225
+/// 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) {
----------------
RKSimon wrote:
> Put the Flag set: on the last line to make it clearer.
ok


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:226
+/// 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)) {
----------------
filcab wrote:
> Alternative name: `propagateIRFlagsToOp` since you're propagating them to that specific op. What do you think?
looks good


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:227
+static void propagateIRFlagsWithOp(Value *I, ArrayRef<Value *> VL, Value *OpValue) {
+  if (auto *VecOp = dyn_cast<Instruction>(I)) {
+    auto *Intersection = cast<Instruction>(OpValue);
----------------
filcab wrote:
> Remove indentation levels like you do further in the function:
> 
> 
> ```
> auto *VecOp = dyn_cast<Instruction>(I);
> if (!VecOp)
>   return;
> // rest of the function
> ```
ok, correct


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:228
+  if (auto *VecOp = dyn_cast<Instruction>(I)) {
+    auto *Intersection = cast<Instruction>(OpValue);
+    const unsigned Opcode = Intersection->getOpcode();
----------------
RKSimon wrote:
> Should we assume this (the cast will assert on failure) or check for it?
> ```
> if (auto *Intersection = dyn_cast<Instruction>(OpValue)) {
> }
> ```
ok


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2858
+      propagateIRFlagsWithOp(V0, EvenScalars, EvenScalars[0]);
+      propagateIRFlagsWithOp(V1, OddScalars, OddScalars[0]);
 
----------------
filcab wrote:
> What will be the difference between the old code and this one?
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.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4585
                                                Pair.first, "bin.extra");
-          propagateIRFlags(VectorizedTree, I);
+          propagateIRFlagsWithOp(VectorizedTree, I, I);
         }
----------------
RKSimon wrote:
> Should the last argument be I[0]?
correct.


https://reviews.llvm.org/D35292