[PATCH] D85794: [llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI]

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

I put a comment on most of them, but just to elaborate: for any assert of the form `assert(!SomeElementCount.Scalable && "invalid number of elements");`, the issue is that "invalid number of elements" could mean literally anything (is it invalid because there are 4 elements? Who knows). Obviously if you look at the expression being asserted, it narrows it down to just scalable = true being invalid. But it doesn't answer the question "why is it invalid?". Is it actually nonsensical? Is it just not implemented? I may have missed some, but if I did it was an oversight.

Overall, this patch is almost there. We just need to get the bike shed painted the correct color. :)



================
Comment at: llvm/include/llvm/Support/TypeSize.h:82
+  bool isVector() const {
+    assert(!(Min == 0 && Scalable) &&
+           "invalid values: zero can not be scalable");
----------------
since we allow zero-element scalable element counts now, this assert should be removed


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:181
+  static VectorizationFactor Disabled() {
+    return {ElementCount::getFixed(1 /*Min*/), 0};
+  }
----------------
NIT: remove /*Min*/, it's unambiguous what this 1 is here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:232
 
-  unsigned BestVF = 0;
+  /// The best number of elements of the vector types used in the
+  /// transformed loop. Zero elements in the BestVF means that
----------------
update this comment


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:317
+static bool hasIrregularType(Type *Ty, const DataLayout &DL, ElementCount VF) {
+  assert(!VF.Scalable && "invalid number of elements");
   // Determine if an array of VF elements of type Ty is "bitcast compatible"
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported".

If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:837
+      : InnerLoopVectorizer(OrigLoop, PSE, LI, DT, TLI, TTI, AC, ORE,
+                            ElementCount::getFixed(1 /*Min*/), UnrollFactor,
+                            LVL, CM, BFI, PSI) {}
----------------
NIT: remove /*Min*/, it's unambiguous what this 1 is here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:874
         !isa<DbgInfoIntrinsic>(Inst)) {
-      auto NewDIL = DIL->cloneByMultiplyingDuplicationFactor(UF * VF);
+      assert(!VF.Scalable && "invalid number of elements");
+      auto NewDIL = DIL->cloneByMultiplyingDuplicationFactor(UF * VF.Min);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1191
+  InstWidening getWideningDecision(Instruction *I, ElementCount VF) {
+    assert(!VF.Scalable && "invalid element count");
+    assert(VF.isVector() && "Expected VF >=2");
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1303
+  bool
+  isScalarWithPredication(Instruction *I,
+                          ElementCount VF = ElementCount::getFixed(1 /*Min*/));
----------------
NIT: remove /*Min*/, it's unambiguous what this 1 is here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1322
+  bool memoryInstructionCanBeWidened(
+      Instruction *I, ElementCount VF = ElementCount::getFixed(1 /*Min*/));
 
----------------
NIT: remove /*Min*/, it's unambiguous what this 1 is here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1327
   /// and shuffles.
-  bool interleavedAccessCanBeWidened(Instruction *I, unsigned VF = 1);
+  bool interleavedAccessCanBeWidened(
+      Instruction *I, ElementCount VF = ElementCount::getFixed(1 /*Min*/));
----------------
NIT: remove /*Min*/, it's unambiguous what this 1 is here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1819
   //        handle a constant vector splat.
+  assert(!VF.Scalable && "invalid number of elements");
   Value *SplatVF = isa<Constant>(Mul)
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1956
     for (unsigned Part = 0; Part < UF; ++Part) {
-      Value *EntryPart =
-          getStepVector(Broadcasted, VF * Part, Step, ID.getInductionOpcode());
+      assert(!VF.Scalable && "invalid number of elements");
+      Value *EntryPart = getStepVector(Broadcasted, VF.Min * Part, Step,
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2140
     // we created for the last vector lane of the Part unroll iteration.
-    unsigned LastLane = Cost->isUniformAfterVectorization(I, VF) ? 0 : VF - 1;
+    assert(!VF.Scalable && "invalid number of elements");
+    unsigned LastLane =
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2288
   unsigned InterleaveFactor = Group->getFactor();
-  auto *VecTy = FixedVectorType::get(ScalarTy, InterleaveFactor * VF);
+  assert(!VF.Scalable && "invalid number of elements");
+  auto *VecTy = VectorType::get(ScalarTy, VF * InterleaveFactor);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2343
   if (Group->requiresScalarEpilogue() && !Cost->isScalarEpilogueAllowed()) {
-    MaskForGaps = createBitMaskForGaps(Builder, VF, *Group);
+    assert(!VF.Scalable && "Invalid number of elements");
+    MaskForGaps = createBitMaskForGaps(Builder, VF.Min, *Group);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2361
           auto *Undefs = UndefValue::get(BlockInMaskPart->getType());
+          assert(!VF.Scalable && "Invalid element count.");
           Value *ShuffledMask = Builder.CreateShuffleVector(
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2391
 
-      auto StrideMask = createStrideMask(I, InterleaveFactor, VF);
+      assert(!VF.Scalable && "Invalid element count");
+      auto StrideMask = createStrideMask(I, InterleaveFactor, VF.Min);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2443
     // Interleave the elements in the wide vector.
+    assert(!VF.Scalable && "invalid element count");
     Value *IVec = Builder.CreateShuffleVector(
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2488
+
+  assert(!VF.Scalable && "invalid number of elements");
+  auto *DataTy = VectorType::get(ScalarDataTy, VF);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2821
+  if (!Cost->foldTailByMasking()) {
+    assert(!VF.Scalable && "invalid number of elements");
     CheckMinIters = Builder.CreateICmp(
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3277
   // times the unroll factor (num of SIMD instructions).
-  Constant *Step = ConstantInt::get(IdxTy, VF * UF);
+  assert(!VF.Scalable && "invalid number of elements");
+  Constant *Step = ConstantInt::get(IdxTy, VF.Min * UF);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3412
                                                        bool &NeedToScalarize) {
+  assert(!VF.Scalable && "invalid number of elements");
   Function *F = CI->getCalledFunction();
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3971
     assert(!IsInLoopReductionPhi && "Unexpected truncated inloop reduction!");
-    Type *RdxVecTy = FixedVectorType::get(RdxDesc.getRecurrenceType(), VF);
+    assert(!VF.Scalable && "invalid number of elements");
+    Type *RdxVecTy = VectorType::get(RdxDesc.getRecurrenceType(), VF);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4296
+                                              ElementCount VF) {
+  assert(!VF.Scalable && "invalid number of elements");
   PHINode *P = cast<PHINode>(PN);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4441
                                            VPTransformState &State) {
+  assert(!VF.Scalable && "invalid number of elements");
   switch (I.getOpcode()) {
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4830
+                                                         ElementCount VF) {
+  assert(!VF.Scalable && "invalid number of elements");
   if (!blockNeedsPredication(I->getParent()))
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5319
 LoopVectorizationCostModel::selectVectorizationFactor(unsigned MaxVF) {
-  float Cost = expectedCost(1).first;
+  float Cost = expectedCost(ElementCount::getFixed(1 /*Min*/)).first;
   const float ScalarCost = Cost;
----------------
NIT: remove /*Min*/, it's unambiguous what this 1 is here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5503
   // Clamp the interleave ranges to reasonable counts.
-  unsigned MaxInterleaveCount = TTI.getMaxInterleaveFactor(VF);
+  assert(!VF.Scalable && "invalid number of elements");
+  unsigned MaxInterleaveCount = TTI.getMaxInterleaveFactor(VF.Min);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5690
     unsigned TypeSize = DL.getTypeSizeInBits(Ty->getScalarType());
-    return std::max<unsigned>(1, VF * TypeSize / WidestRegister);
+    assert(!VF.Scalable && "invalid number of elements");
+    return std::max<unsigned>(1, VF.Min * TypeSize / WidestRegister);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5918
     // computing the scalarization overhead.
-    unsigned ScalarCost = VF * getInstructionCost(I, 1).first;
+    assert(!VF.Scalable && "invalid number of elements");
+    unsigned ScalarCost =
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5928
+          APInt::getAllOnesValue(VF.Min), true, false);
+      assert(!VF.Scalable && "invalid number of elements");
+      ScalarCost +=
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5966
+LoopVectorizationCostModel::expectedCost(ElementCount VF) {
+  assert(!VF.Scalable && "invalid number of elements");
   VectorizationCostTy Cost;
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6049
+         "Scalarization cost of instruction implies vectorization.");
+  assert(!VF.Scalable && "invalid number of elements");
   Type *ValTy = getMemInstValueType(I);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6163
   unsigned InterleaveFactor = Group->getFactor();
-  auto *WideVecTy = FixedVectorType::get(ValTy, VF * InterleaveFactor);
+  assert(!VF.Scalable && "invalid number of elements");
+  auto *WideVecTy = VectorType::get(ValTy, VF * InterleaveFactor);
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6275
+void LoopVectorizationCostModel::setCostBasedWideningDecision(ElementCount VF) {
+  assert(!VF.Scalable && "invalid number of elements");
+  if (VF.isScalar() && !VF.Scalable)
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6462
       // Return cost for branches around scalarized and predicated blocks.
+      assert(!VF.Scalable && "invalid number of elements");
       auto *Vec_i1Ty =
----------------
The assert message is unclear. Change it to something like "scalable vectors not yet supported". If this function does not make sense for scalable vectors, then we should explicitly operate on FixedVectorType


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6604
       if (Decision == CM_Scalarize)
-        Width = 1;
+        Width = ElementCount::getFixed(1 /*Min*/);
     }
----------------
NIT: remove /*Min*/, it's unambiguous what this 1 is here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6936
+  VPTransformState State{
+      BestVF.getValue(),      BestUF, LI,         DT, ILV.Builder,
+      ILV.VectorLoopValueMap, &ILV,   CallbackILV};
----------------
For `BestVF.getValue()`, I would use `Optional<T>::operator*`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794