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

[Igor Kirillov via Phabricator via llvm-commits]

igor.kirillov marked 3 inline comments as done.
igor.kirillov added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:375
+      // IntermediateStore is always the last store in the loop.
+      IntermediateStore = SI;
+      continue;
----------------
fhahn wrote:
> Do we have to make sure that the stored value is feeding the phi again and not an earlier value in the cycle?
Yes! I added this check and also a test case (reduc_store_not_final_value)


================
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:
> 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235