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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

This looks really good now @kmclaughlin! Thanks for strengthening the `getReductionOpChain` code and adding a plethora of negative chained reduction tests. :) I just have a few more minor comments, but I think it's almost good to go!



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1111
+    Instruction *Chain = nullptr;
+    if (Inc0 && Inc0 == Phi)
+      Chain = Inc1;
----------------
nit: Before merging could you simplify this to something like

  if (Inc0 == Phi)
    Chain = Inc1;
  else if (Inc1 == Phi)
    Chain = Inc0;
  else
    return {};

Thanks!


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1129
 
-  // Check that the Phi has one (or two for min/max) uses.
-  if (!Phi->hasNUses(ExpectedUses))
+  // Check that the Phi has one (or two for min/max or conditional) uses.
+  if (!Phi->hasNUses(ExpectedUses + ExtraPhiUses))
----------------
nit: I think for conditional min/max reductions there would be three uses. Perhaps you can reword as something like:

  // Check that the Phi has one (or two for min/max) uses for unconditional reductions, plus
  // an extra use for conditional reductions.

What do you think?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8597
+  // For in-loop reductions, we do not need to create an additional select.
+  if (Phi->getNumIncomingValues() == 2) {
+    PHINode *In0 = dyn_cast_or_null<PHINode>(Operands[0]->getUnderlyingValue());
----------------
It might be worth adding more asserts here, i.e. assert that if any operand is an in-loop reduction that we only have two incoming values, as well as asserting that there is only one in-loop reduction operand?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8613
   SmallVector<VPValue *, 2> OperandsWithMask;
   unsigned NumIncoming = Phi->getNumIncomingValues();
 
----------------
nit: If you move this higher up I think you can reuse it, i.e.

  unsigned NumIncoming = Phi->getNumIncomingValues();

  // For in-loop reductions, we do not need to create an additional select.
  if (Phi->getNumIncomingValues() == 2) {



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

https://reviews.llvm.org/D117580