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

[Kerry McLaughlin via Phabricator via llvm-commits]

kmclaughlin added a comment.

Thank you for reviewing these changes, @david-arm!



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1058
     ExpectedUses = 2;
+  unsigned ExpectedPhiUses = ExpectedUses;
 
----------------
david-arm wrote:
> 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.
Hi @david-arm, I think it's simpler to not support a chain of conditional reductions here to begin with. I've added some tests to reduction-inloop-cond.ll for various loops containing chained conditional reductions, including @uncond_cond where Phi does not have the expected number of uses.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1117
+
   Instruction *Cur = getNextInstruction(Phi);
 
----------------
david-arm wrote:
> 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!!
This worked for some cases, but you're right that it was incorrect for the LoopExitInstr to be returned first. I've rewritten getNextInstruction as you suggested, so that we look to the next user if a Phi node is found and added a test case (@simple_chained_rdx) with two reduction operations between the Phi & LoopExitInst.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1121
+  // value found earlier.
+  if (dyn_cast<PHINode>(Cur) == LoopExitInstr)
+    Cur = RdxInstr;
----------------
david-arm wrote:
> Is there ever a case where this check fails despite LoopExitInstr being a PHI node?
If there is a phi node in the chain between Phi and the LoopExitInstr then this check could fail, though `isCorrectOpcode(Cur)` will return false. I've added a test for this to reduction-inloop-cond.ll.
I've removed this check though as it isn't necessary after the changes to getNextInstruction.


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

https://reviews.llvm.org/D117580