[PATCH] D110235: [LoopVectorize] Support reductions that store intermediary result

[Igor Kirillov via Phabricator via llvm-commits]

igor.kirillov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9335
+      // is no need to create a recipe for them.
+      StoreInst *SI;
+      if ((SI = dyn_cast<StoreInst>(&I)) &&
----------------
fhahn wrote:
> I think it would also be good to include at least some information in the VPlan that the store is handled as part of the reduction. Perhaps `VPReductionRecipe` should print if the result is stored after the loop? Please add a test case to `vplan-printing.ll`
I'm not sure how to display this information properly. This is how output of `VReductionRecipe::print` looks like now:

 REDUCE ir<%red.next> = ir<%red> + fast reduce.fadd (ir<%lv>)

I could add something to the end but it doesn't seem to fit there. Also I have not found any proper `V.*Recipe` where I could place something like `DELETE store .*`. 


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction.ll:469
+; int sum = 0;
+; for(i=0..N) {
+;   sum += src[i];
----------------
fhahn wrote:
> FWIW for such compact IR test cases, the pseudo code doesn't add much value in my personal opinion. Better to strive to make the tests as readable/compact as possible and have a comment explaining what it tests when needed.
When I look at pseudo-code I immediately understand what it is about, whereas looking at IR takes at least 30 seconds of cognitive exertions. And it is also easier to see the difference between and purpose of all those quite similar tests. But I delete it, of course, if you insist :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235