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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:902
+      if (isRecurringInvariantStore(SI, &IsPredicated)) {
+        if (IsPredicated)
+          UnhandledStores.push_back(SI);
----------------
Hi @igor.kirillov, I think you're missing a test case here. Suppose we have two stores in the loop to the same invariant address. If the first store is predicated, but the second isn't we should still vectorise because the second store wins and the first one can be removed. At least the code here suggests that. I couldn't find any test that exercised this code path.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:910
+        }
+      } else if (!isUniform(SI->getValueOperand()))
+        UnhandledStores.push_back(SI);
----------------
I think you might be missing a test case for this. Can you make sure this code path is exercised please by your existing tests please?


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction.ll:477
+
+; CHECK: vector.body:
+; CHECK-NOT: store i32 %{{[0-9]+}}, i32* %arrayidx
----------------
Can you put all the CHECK lines in the same place near the top of the function please to be consistent with the existing tests?


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction.ll:517
+  %1 = load float, float* %arrayidx1, align 4
+  %add = fadd float %0, %1
+  store float %add, float* %arrayidx, align 4
----------------
Hi @igor.kirillov, this doesn't look right. The test is called `@reduc_store_fadd_fast`, but there is not `fast` keyword on the `fadd` instruction. I don't think we should even be vectorising this because it requires ordered reductions, which you haven't enabled for this test. Can you change the name of this to `@reduc_store_fadd_ordered` and investigate why we are vectorising this?

Can you also add a separate test called `@reduc_store_fadd_fast` that actually has the `fast` keyword too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110235