[PATCH] D143465: [LoopVectorize] Vectorize the reduction pattern of integer min/max with index.

[Ramkumar Ramachandra via Phabricator via llvm-commits]

artagnon added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:424
+        UserRecurInstr = Cur;
+        // TODO: Call AddReductionVar here?
+
----------------
Why?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:433-435
+        if (match(UserRecurInstr,
+                  m_Select(m_OneUse(m_Cmp()), m_Value(), m_Value())))
+          --NumCmpSelectPatternInst;
----------------
If you separate out the MinMaxIdx pattern into its own function, we can check `NumCmpSelectPatternInst` for it separately.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:879-908
+  case CmpInst::ICMP_SLT:
+  case CmpInst::ICMP_ULT:
+    // %mmphi <  %0
+    UpdateSide = isMaxRecurrenceKind(MinMaxKind);
+    ExpectedIdxRK = isMaxRecurrenceKind(MinMaxKind) ? RecurKind::MinMaxFirstIdx
+                                                    : RecurKind::MinMaxLastIdx;
+    break;
----------------
This is a bit cryptic: would you consider adding more `RecurKind`s to make this less cryptic?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:925-926
+  InductionDescriptor ID;
+  if (!InductionDescriptor::isInductionPHI(IdxUpdatePhi, Loop, SE, ID))
+    return InstDesc(false, I);
+
----------------
Can we avoid the expensive call to `isInductionPHI()` by checking that the `SCEVAddRec` is a `SCEVConstant`?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1292-1296
   case RecurKind::SelectIVICmp:
   case RecurKind::SelectIVFCmp:
+  case RecurKind::MinMaxFirstIdx:
+  case RecurKind::MinMaxLastIdx:
     return getRecurrenceIdentity(RecurKind::SMax, Tp, FMF);
----------------
Why not merge this with the `RecurKind::SMax` case?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1326-1327
+  // TODO: maybe new FMinMaxFirstIdx/ FMinMaxLastIdx
+  case RecurKind::MinMaxFirstIdx:
+  case RecurKind::MinMaxLastIdx:
     return Instruction::ICmp;
----------------
Rename these to `IMinMaxFirstIdx` and `IMinMaxLastIdx`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1027
 
+  // Second comfirm the incomplete reductions
+  for (auto R : Reductions) {
----------------
Typo: comfirm.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4064-4067
+      // FIXME: Not sure use FCMP_OEQ is right or not.
+      CmpInst::Predicate MaskPred =
+          (ReducedPartRdx->getType()->isFloatingPointTy()) ? CmpInst::FCMP_OEQ
+                                                           : CmpInst::ICMP_EQ;
----------------
`RdxDesc.isOrdered()` can help you pick between `FCMP_OEQ` and `FCMP_UEQ`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143465