[PATCH] D98435: [LoopVectorize] Add strict in-order reduction support for fixed-width vectorization

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:215
+    Exit = cast<Instruction>(ChainVal);
+    IsOrdered = Exit->getOpcode() == Instruction::FAdd &&
+                !cast<FPMathOperator>(Exit)->hasAllowReassoc();
----------------
HI @kmclaughlin, I think you can combine the two `IsOrdered =` statements into one by declaring the variable below the `if (auto *EIP = ...) { ... }` block.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4360
   setDebugLocFromInst(Builder, LoopMiddleBlock->getTerminator());
   {
+    if (Cost->isInLoopReduction(Phi) && useOrderedReductions(RdxDesc))
----------------
nit: I think you can actually now just kill the `{` brace here and the other one `}` at line 4377. That wil avoid the additional indentation.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9135
+    if (IsOrdered) {
+      Value *C = ConstantInt::get(
+          IntegerType::getInt1Ty(State.Builder.getContext()), 1);
----------------
nit: I think you can just do:

  Value *C = Builder.getInt1(1);


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9150
                          NewRed, PrevInChain);
     } else {
+      if (IsOrdered)
----------------
nit: You could avoid some of the extra indentation below if you changed the `} else {` to something like:

  } else if (IsOrdered)
    NextInChain = NewRed;
  } else {
    ...
  }


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/strict-fadd.ll:118
+
+define float @fadd_conditional_rdx(float* noalias nocapture readonly %a, float* noalias nocapture readonly %b, i64 %n) {
+; CHECK-LABEL: @fadd_conditional_rdx
----------------
fhahn wrote:
> I don't think this test is testing what you intended it to. The condition `> 0.5f` is in the loop pre-header. I think it should be in the loop body, so it tests generating the correct mask? (The C source here is a bit misleading IMO)
Hi @fhahn, I suspect it's because when compiled with clang the condition is hoisted out before we reach the vectoriser, but you're right I think that the condition should be in the loop. At the moment this test is really the same as `@fadd_strict`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98435