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

[Igor Kirillov via Phabricator via llvm-commits]

igor.kirillov added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-with-invariant-store.ll:170
+; CHECK: vector.body:
+; CHECK-NOT: store i32 %{{[0-9]+}}, i32* %gep.dst
+; CHECK: middle.block:
----------------
fhahn wrote:
> fhahn wrote:
> > igor.kirillov wrote:
> > > fhahn wrote:
> > > > I think it would be good to check the full reduction cycle & store here. I don't think this test case is handled correctly at the moment as the IR seems out of sync with the pseudo code (which is why personally I think the C pseudo code is a bit distracting)
> > > > 
> > > > Note that in the IR the reduction cycle is `%sum -> sum.1 -> %sum` and not `%sum -> %sum.1 -> %sum.2 -> %sum`. When this gets vectorized, the final value written to dst[42] is the final value of %sum.1, not %sum.2 as it should be I think. 
> > > The IR was incorrect actually because of the error I made during renaming and the pseudo-code was showing my original plot for the test-case. Luckily it helped to expose a bug, so I added an extra check to `IVDescriptors.cpp` and now `reduc_store_not_final_value` function is testing this case (when we have IntermediateStore but no ExitInstruction and value stored is not actually a final value).
> > I think it would still be good to test that the full reduction cycle is generated correctly (probably worth checking the full vector body + middle.block), at least for some of the tests in the file, to make sure the full sequence is generated correctly.
> > The IR was incorrect actually because of the error I made during renaming and the pseudo-code was showing my original plot for the test-case. 
> 
> This seems to indicate that the pseudo code may be distracting (: 
@fhahn Added full checks for `reduc_store` and this function. The code generated is overscalarized for `reduc_store_inside_unrolled` but I checked and without invariant stores it is the same. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235