[SLP] AdjustExtractsCost - remove redundant subvector extraction code

[RKSimon]

Summary

I noticed this while investigating replacing the getVectorInstrCost(Instruction::ExtractElement) calls with getScalarizationOverhead instead.

This code should be helping improve the costs of extracting an element from subvectors created during legalization, but removing it seems to have no effect - I'm guessing the getVectorInstrCost/getScalarizationOverhead improvements may have made it redundant?

[ABataev]

I rather doubt that it is redundant. This code is for the case, where the buildvector of extractelements results in the different vector size rather than the original size of vector(say, we extract elements from <4x i32> but the the resulting vector is <8xi32> or <2 x i32>). In this case we need to take the inser/extract subvector cost, especially if we insert somewhere in the middle of the vector register. Instead, we need to adjust/fix the idx for TargetTransformInfo::SK_InsertSubvector shuffle kind, currently it is too optimistic. I used 0 here because TTI cost model was not optimal and in many cases we ended up with scalarization overhead rather than actual shuffles.

[RKSimon]

OK, redundant probably isn't correct - but we still have the situation that removing this code has no effect on tests any more.

It appears to have been added in D99980.

[ABataev]

In D124769#3485656, @RKSimon wrote:

OK, redundant probably isn't correct - but we still have the situation that removing this code has no effect on tests any more.

It appears to have been added in D99980.

Its just the tests we have are not affected anymore but some real code is still affected. The test codebase is not full, many cases are missing. Removing this code may cause ineffective vectorization in some corner cases. We need to keep this and fix

Cost += TTIRef.getShuffleCost(TargetTransformInfo::SK_InsertSubvector,
                                      VecTy, None, 0, EEVTy); <---- 0 is not very correct (too optimistic).

instead.

[vporpo]

Somewhat related to this patch: For cost modeling changes it would probably be helpful to have tests with cost values in them. Because some of these changes may not directly affect the generated IR, but would show up in the cost diff. For example something like: https://reviews.llvm.org/D124802 . Any thoughts?

[RKSimon]

I'll come back to this - ideally we'd avoid handling the subvector cost adjustments in SLP and let getScalarizationOverhead deal with it.

[ABataev]

In D124769#3498914, @RKSimon wrote:

I'll come back to this - ideally we'd avoid handling the subvector cost adjustments in SLP and let getScalarizationOverhead deal with it.

I think this is possible, but need to reimplement the whole AdjustExtractsCost, need to use getScalarizationOverhead instead of getVectorInstrCostr.

[RKSimon]

In D124769#3498929, @ABataev wrote:

In D124769#3498914, @RKSimon wrote:

I'll come back to this - ideally we'd avoid handling the subvector cost adjustments in SLP and let getScalarizationOverhead deal with it.

I think this is possible, but need to reimplement the whole AdjustExtractsCost, need to use getScalarizationOverhead instead of getVectorInstrCostr.

D125527 improves the duplicated subvector extraction costs on x86

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

InstructionCost BoUpSLP::getEntryCost(const TreeEntry *E,
   unsigned EntryVF = E->getVectorFactor();
   auto *FinalVecTy = FixedVectorType::get(VecTy->getElementType(), EntryVF);
 
   bool NeedToShuffleReuses = !E->ReuseShuffleIndices.empty();
   // FIXME: it tries to fix a problem with MSVC buildbots.
   TargetTransformInfo &TTIRef = *TTI;
   auto &&AdjustExtractsCost = [this, &TTIRef, CostKind, VL, VecTy,
                                VectorizedVals, E](InstructionCost &Cost) {
-    DenseMap<Value *, int> ExtractVectorsTys;
     SmallPtrSet<Value *, 4> CheckedExtracts;
     for (auto *V : VL) {
       if (isa<UndefValue>(V))
         continue;
       // If all users of instruction are going to be vectorized and this
       // instruction itself is not going to be vectorized, consider this
       // instruction as dead and remove its cost from the final cost of the
       // vectorized tree.
       // Also, avoid adjusting the cost for extractelements with multiple uses
       // in different graph entries.
       const TreeEntry *VE = getTreeEntry(V);
       if (!CheckedExtracts.insert(V).second ||
           !areAllUsersVectorized(cast<Instruction>(V), VectorizedVals) ||
           (VE && VE != E))
         continue;
       auto *EE = cast<ExtractElementInst>(V);
       Optional<unsigned> EEIdx = getExtractIndex(EE);
       if (!EEIdx)
         continue;
       unsigned Idx = *EEIdx;
-      if (TTIRef.getNumberOfParts(VecTy) !=
-          TTIRef.getNumberOfParts(EE->getVectorOperandType())) {
-        auto It =
-            ExtractVectorsTys.try_emplace(EE->getVectorOperand(), Idx).first;
-        It->getSecond() = std::min<int>(It->second, Idx);
-      }
       // Take credit for instruction that will become dead.
       if (EE->hasOneUse()) {
         Instruction *Ext = EE->user_back();
         if ((isa<SExtInst>(Ext) || isa<ZExtInst>(Ext)) &&
             all_of(Ext->users(),
                    [](User *U) { return isa<GetElementPtrInst>(U); })) {
           // Use getExtractWithExtendCost() to calculate the cost of
           // extractelement/ext pair.
           Cost -=
               TTIRef.getExtractWithExtendCost(Ext->getOpcode(), Ext->getType(),
                                               EE->getVectorOperandType(), Idx);
           // Add back the cost of s|zext which is subtracted separately.
           Cost += TTIRef.getCastInstrCost(
               Ext->getOpcode(), Ext->getType(), EE->getType(),
               TTI::getCastContextHint(Ext), CostKind, Ext);
           continue;
         }
       }
       Cost -= TTIRef.getVectorInstrCost(Instruction::ExtractElement,
                                         EE->getVectorOperandType(), Idx);
     }
-    // Add a cost for subvector extracts/inserts if required.
-    for (const auto &Data : ExtractVectorsTys) {
-      auto *EEVTy = cast<FixedVectorType>(Data.first->getType());
-      unsigned NumElts = VecTy->getNumElements();
-      if (Data.second % NumElts == 0)
-        continue;
-      if (TTIRef.getNumberOfParts(EEVTy) > TTIRef.getNumberOfParts(VecTy)) {
-        unsigned Idx = (Data.second / NumElts) * NumElts;
-        unsigned EENumElts = EEVTy->getNumElements();
-        if (Idx + NumElts <= EENumElts) {
-          Cost +=
-              TTIRef.getShuffleCost(TargetTransformInfo::SK_ExtractSubvector,
-                                    EEVTy, None, Idx, VecTy);
-        } else {
-          // Need to round up the subvector type vectorization factor to avoid a
-          // crash in cost model functions. Make SubVT so that Idx + VF of SubVT
-          // <= EENumElts.
-          auto *SubVT =
-              FixedVectorType::get(VecTy->getElementType(), EENumElts - Idx);
-          Cost +=
-              TTIRef.getShuffleCost(TargetTransformInfo::SK_ExtractSubvector,
-                                    EEVTy, None, Idx, SubVT);
-        }
-      } else {
-        Cost += TTIRef.getShuffleCost(TargetTransformInfo::SK_InsertSubvector,
-                                      VecTy, None, 0, EEVTy);
-      }
-    }
   };
   if (E->State == TreeEntry::NeedToGather) {
     if (allConstant(VL))
       return 0;
     if (isa<InsertElementInst>(VL[0]))
       return InstructionCost::getInvalid();
     SmallVector<int> Mask;
     SmallVector<const TreeEntry *> Entries;