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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @igor.kirillov, thanks for all the changes and the patch looks good now! I just had a couple of minor comments.



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:457
+        else {
+          if (auto *SI = dyn_cast<StoreInst>(UI)) {
+            // Reduction variable chain can only be stored somewhere but it
----------------
nit: Could you remove an extra level of indentation here before merging the patch?

i.e. this can be written like this:

  if (isa<PHINode>(UI))
    PHIs.push_back(UI);
  else if (auto *SI = dyn_cast<StoreInst>(UI)) {
    ...
  } else
    NonPHIs.push_back(UI);


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:974
+                                                          bool *IsPredicated) {
+  bool FoundMatchingRecurrence = false;
+  for (auto &Reduction : Reductions) {
----------------
nit: I think you can you might be able to simplify the code here by removing `FoundMatchingRecurrence `. Then, lower down instead of the `break` you can just do

      if (DSI && (DSI == SI)) {
        *IsPredicated = blockNeedsPredication(DSI->getParent());
        return true;
      }

and at the bottom of the function just do:

  return false;



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235