[PATCH] D117213: [LoopVectorize] Add tests with reductions that are stored in invariant address

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! A few additional suggestions inline.



================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-with-invariant-store.ll:1
+; RUN: opt < %s  -loop-vectorize -force-vector-interleave=1 -force-vector-width=4 -dce -instcombine -S | FileCheck %s
+
----------------
Requiring other passes makes the test more fragile. At least in the current version, there should be no need to require extra cleanup passes.

Also, it might be good to use the new pass manager syntax (`-passes=""`)


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-with-invariant-store.ll:9
+; int sum = 0;
+; for(i=0..N) {
+;   sum += src[i];
----------------
FWIW I don't think the pseudo code here adds much, especially because there is a textual comment. and the IR is very compact. Just a personal preference/opinion though.


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-with-invariant-store.ll:318
+for.body:
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+  %sum = phi i32 [ 0, %entry ], [ %sum.1, %for.body ]
----------------
might be better to just drop `indvars.`


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-with-invariant-store.ll:328
+
+for.cond.cleanup:
+  %sum.lcssa = phi i32 [ %sum.1, %for.body ]
----------------
nit: might be slightly simpler to call this exit.


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-with-invariant-store.ll:334
+}
+
----------------
stray new line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117213