[PATCH] D150851: [LoopVectorize] Vectorize select-cmp reduction pattern for increasing integer induction variable

[Mel Chen via Phabricator via llvm-commits]

Mel-Chen added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:364
 /// between two values: 1) an initial PHI start value, and 2) a loop invariant
-/// value. This function uses \p LoopExitInst to determine 2), which we then use
-/// to select between \p Left and \p Right. Any lane value in \p Left that
-/// matches 2) will be merged into \p Right.
+/// value and increasing loop induction variable. This function uses \p
+/// LoopExitInst to determine 2), which we then use to select between \p Left
----------------
shiva0217 wrote:
> or an increasing loop induction variable?
Good find! Will correct it.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1162
 
+Value *llvm::createSentinelValueHandling(IRBuilderBase &Builder,
+                                         const TargetTransformInfo *TTI,
----------------
shiva0217 wrote:
> It might be worth to have a comment to describe that the SelectIVICmp and SelectIVFCmp code generation will use Identity value to determine the if condition in the following case has ever been true.
> 
>   int r = 331;
>   for (int i = 0; i < n; i++)
>     if (src[i] > 111)
>       r = i;
> 
> When the reduction value(Rdx) equal to the Identity value(Iden), it reveals the condition never been true. So it will select the InitVal.
> 
> It might be the reason that in IsIncreasingLoopInduction, the function will check the IV start value to avoid the IV overlapping the Identity value.
Sure, will do.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4003
+    ReducedPartRdx =
+        createSentinelValueHandling(Builder, TTI, RdxDesc, ReducedPartRdx);
+
----------------
shiva0217 wrote:
> Could the function rename to createSelectIVCmpTargetReduction and be called from createSelectCmpTargetReduction?
> Perhaps CreateIntMaxReduce can be moved to the function?
I'm afraid I won't be able to meet this requirement. Placing `createSentinelValueHandling` in this position is for handling the case when the vector width is 1. You could refer to CHECK-VF1IC4 in the test cases and focus on the `middle.block`. In implementation, VF1IC4 doesn't call `createTargetReduction`, but `ReducedPartRdx` still need to be did the sentinel value fixing.

However, perhaps we can create a new bool function for `RK == RecurKind::SelectIVICmp || RK == RecurKind::SelectIVFCmp`. This will most likely expand further and cause the if-condition to become too long.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1307
     }
+  } else if (RK == RecurKind::SelectIVICmp || RK == RecurKind::SelectIVFCmp) {
+    StartV = Iden = RdxDesc.getRecurrenceIdentity(RK, VecTy->getScalarType(),
----------------
shiva0217 wrote:
> Perhaps a comment to describe that SelectIVICmp and SelectIVFCmp will initial the reduction PHI with Iden and createSentinelValueHandling will use Iden to determine the if condition in the loop has ever been true?
Sure, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150851