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

[Mel Chen via Phabricator via llvm-commits]

Mel-Chen marked 2 inline comments as done.
Mel-Chen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:55-57
+  SelectIVICmp, ///< Integer select(icmp(),x,y) where one of (x,y) is increasing
+                ///< loop induction PHI
+  SelectIVFCmp, ///< Integer select(fcmp(),x,y) where one of (x,y) is increasing
----------------
artagnon wrote:
> Maybe have a `max` in the name to make it clear that we only do max-reductions?
It has some functional overlap with max reduction, but it is not exactly the same as max reduction.
If we were to rename it, the change I suggest is:
SelectICmp --> SelectInvICmp
SelectFCmp --> SelectInvFCmp
SelectIVICmp --> SelectIncIVICmp
SelectIVFCmp --> SelectIncIVFCmp
This will also make it easier for you to expand it in the future.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:613-633
 // We are looking for loops that do something like this:
 //   int r = 0;
 //   for (int i = 0; i < n; i++) {
 //     if (src[i] > 3)
 //       r = 3;
 //   }
 // where the reduction value (r) only has two states, in this example 0 or 3.
----------------
artagnon wrote:
> Please update these comments.
Nice catch! Will update it.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:661
 
+  auto IsIncreasingLoopInduction = [&SE, &Loop](Value *V) {
+    auto *Phi = dyn_cast<PHINode>(V);
----------------
artagnon wrote:
> Why not split this out into its own function?
I'm also think about this because I personally feel that this function is a bit long. 
Do you think it's better to put it here as a static function, or put it in another file?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:663-671
+    if (!Phi)
+      return false;
+
+    if (!SE)
+      return false;
+
+    InductionDescriptor ID;
----------------
artagnon wrote:
> We should forbid these cases in the first place instead of checking them.
I do not understand. I believe this series of checks already forbids these cases.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:674
+    const SCEVAddRecExpr *AR = cast<SCEVAddRecExpr>(SE->getSCEV(Phi));
+    if (!AR->hasNoSignedWrap())
+      return false;
----------------
artagnon wrote:
> Mel-Chen wrote:
> > fhahn wrote:
> > > Do we need to distinguish here between no signed/no unsigned wrap and then chose `smax/umax` during codegen?
> > That's a good point. Implementing the patch for both signed and unsigned induction variables can be challenging in practice. 
> > The current patch only focuses on the signed IV because in most applications, we often encounter induction variables in the form of IV {0, +, step}. When the IV is signed, we can use -1 or the minimum value of the signed data type as a sentinel value. However, when the IV is unsigned, we don't have a value smaller than 0 to use. This doesn't mean that unsigned IV cannot be vectorized, but rather they require additional handling and a more refined approach.
> > 
> > Of course, if an unsigned IV is {1, +, step}, we can directly use the method implemented in this patch. However, such cases are less common, so we decided to focus on handling signed IV first.
> Doesn't the `InstDesc` tell you if it's signed or unsigned?
No, it does not provide signed or unsigned information.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:677-679
+    ConstantInt *IVStartValue = dyn_cast<ConstantInt>(ID.getStartValue());
+    if (!IVStartValue || IVStartValue->isMinSignedValue())
+      return false;
----------------
artagnon wrote:
> Why is this necessary? Even if it isn't the min signed value, we should codegen just fine, as the codegen just involves applying a mask and applying max/min-reduce.
This is complicated, let me explain why this pattern requires this check.
It is related to our goals: 1) We want to achieve the reduction with one reduce intrinsic. 2) We want to use a static sentinel value.  

About 1), , I understand that even without checking the boundaries or ensuring the select operand is increasing or decreasing, we could still vectorize it by using two reductions. However, the most common format we encounter for induction variables is {0, +, 1}. If this IV is signed, we can accomplish the reduction in one reduction. Therefore, this is based on performance considerations.

As for 2), it follows the decision made in 1), which requires us to have a sentinel value. In the case of an increasing IV, the only restriction on the sentinel value is that it must be less than the start value of the IV. In the case of {0, +, 1}, the sentinel value can be -1 or a smaller value, i.e. dynamic sentinel value. However, for easier implementation, we finally decided to use a static sentinel value, which is the minimum value of the data type.

Additional description: If we were to use a dynamic sentinel value, it would involve checking whether the data type can represent `IV start value - step` (in the case of {0, +, 1}, it would be -1). However, within the LLVM framework, I currently don't have a way to implement a version with a dynamic sentinel value, which is a room for potential improvement.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:681-682
+
+    const SCEV *Step = ID.getStep();
+    return SE->isKnownPositive(Step);
+  };
----------------
artagnon wrote:
> `getStepRecurrence()` from the SCEV AddRec, instead of relying on `isInductionPhi()`?
Why? What are the shortcomings that make you think `isInductionPHI` should not be used?
In my mind, better to rely on the existing function rather than trying to reimplement it.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1089
+    // TODO: Decreasing induction need fix here
+    return Builder.CreateIntMaxReduce(Src, true);
+  default:
----------------
artagnon wrote:
> Personally, I prefer straight-line codegen as I've done, but I'm probably biased.
I don't get it. Only one I need is creating a signed max reduce. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150851