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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:57
+                ///< loop induction PHI
+  SelectIVFCmp, ///< Integer select(fcmp(),x,y) where one of (x,y) is increasing
+                ///< loop induction PHI
----------------
To keep the diff more compact, could you split the FP handling off? It also looks like codegen is at least not tested for the FP case?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:674
+    const SCEVAddRecExpr *AR = cast<SCEVAddRecExpr>(SE->getSCEV(Phi));
+    if (!AR->hasNoSignedWrap())
+      return false;
----------------
Do we need to distinguish here between no signed/no unsigned wrap and then chose `smax/umax` during codegen?


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1088
+  case RecurKind::SelectIVFCmp:
+    // TODO: Decreasing induction need fix here
+    return Builder.CreateIntMaxReduce(Src, true);
----------------
What does this mean? The current patch only handles increasing inductions, so there's no mis-compile that needs fixing here( which the comment kind-of implies)?


================
Comment at: llvm/test/Transforms/LoopVectorize/iv-select-cmp-no-wrap.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -passes=loop-vectorize -force-vector-interleave=1 -force-vector-width=4 -S < %s | FileCheck %s --check-prefix=CHECK
----------------
Could you submit the tests separately?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150851