[PATCH] D69563: [LV] Strip wrap flags from vectorized reductions

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Add test(s) having multiple wrapping reduction instructions.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:382
 
+/// Return set of instructions comprising reduction defined by \p Phi
+static void collectReduInstructions(PHINode *Phi, Loop *L,
----------------
End sentence with period.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:383
+/// Return set of instructions comprising reduction defined by \p Phi
+static void collectReduInstructions(PHINode *Phi, Loop *L,
+                                    SmallPtrSetImpl<Instruction *> &Result) {
----------------
collectReduInstructions >> collectReductionInstructions


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:394
+      Instruction *UI = cast<Instruction>(U);
+      if (UI == Phi || !L->contains(UI->getParent()))
+        continue;
----------------
Phi was initially inserted into Result, so the check if UI==Phi is covered by the check if Result.insert(UI).second. Can fold into 
```
if (L->contains(UI->getParent()) && Result.insert(UI).second)
  Worklist.push_back(UI);
```


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3724
   Value *LoopVal = Phi->getIncomingValueForBlock(Latch);
+  SmallPtrSet<Instruction *, 4> ReduList;
+  collectReduInstructions(Phi, OrigLoop, ReduList);
----------------
ReduList >> ReductionInstructions


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3727
   for (unsigned Part = 0; Part < UF; ++Part) {
+    // wrap flags might become invalid after vectorization, clear them
+    for (Instruction *I : ReduList)
----------------
might become >> are in general
Start sentence with capital (Wrap) and end with period.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3729
+    for (Instruction *I : ReduList)
+      if (isa<OverflowingBinaryOperator>(I)) {
+        Value *V = getOrCreateVectorValue(I, Part);
----------------
Better interchange to loop over all Parts inside. 
Can then potentially do "if (!isa<OverflowingBinaryOperator>(I)) continue;" if preferred.


================
Comment at: llvm/test/Transforms/LoopVectorize/nuw.ll:2
+; RUN: opt %s -loop-vectorize -force-vector-interleave=2 -force-vector-width=4 -S | FileCheck %s
+; ModuleID = 'vec.ll'
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:1"
----------------
Mention PR43828, possibly in file or test name.

As interleaving is set to 2, check both sub's.


================
Comment at: llvm/test/Transforms/LoopVectorize/nuw.ll:4
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:1"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
Tests that use x86 targets should be placed under LoopVectorize/X86 directory.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69563/new/

https://reviews.llvm.org/D69563