[PATCH] D117580: [LoopVectorize] Support conditional in-loop vector reductions

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

This looks like a great addition to the vectoriser @kmclaughlin and nice to see us vectorising conditional reductions! I'm just a little concerned about the extra  complexity we're adding when I'm not sure the patch currently supports chained reductions where at least one of them is conditional. If it's trivial to add support for chained reductions then that's great! Otherwise, perhaps it might be simpler initially to say if there is a conditional reduction we don't support them being chained? It might allow you to rewrite the code in a simpler way.



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1058
     ExpectedUses = 2;
+  unsigned ExpectedPhiUses = ExpectedUses;
 
----------------
Hi @kmclaughlin, I'm not sure if I can see tests that exercise the case where ExpectedUses != ExpectedPhiUses and ExpectedUses is actually used, i.e. in the while loop below:

  while (Cur != RdxInstr) {
    ...

Is it possible to add test cases for this? Alternatively, as a first implementation we could decide not to support chained reductions where one of them is conditional? In that way, it might be possible to simplify the code.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1117
+
   Instruction *Cur = getNextInstruction(Phi);
 
----------------
I'm a bit surprised that `getNextInstruction(Phi)` returns `LoopExitInstr` here because there are instructions in-between the reduction phi and the loop exit phi, i.e. the reduction operation in the if block. Although I do believe this is what you're seeing and the code seems to work!!


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1121
+  // value found earlier.
+  if (dyn_cast<PHINode>(Cur) == LoopExitInstr)
+    Cur = RdxInstr;
----------------
Is there ever a case where this check fails despite LoopExitInstr being a PHI node?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117580