[SLP] Fix "gathering" of vector values

[anton-afanasyev]

Summary

For rare exceptional case vector tree node (insertelements for now only)
is marked as NeedToGather, this case is processed by patch. Follow-up
of D98714 to fix bug reported here https://reviews.llvm.org/D98714#2764135.

[xbolva00]

Add testcase?

[anton-afanasyev]

Add testcase (command crashed before)

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

3693–3695

Not sure we should return 0 here, maybe better to return some max cost, like INT_MAX?

4597–4599

Is this correct? Need to return the last insertelement instruction in the sequence

llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector-inseltpoison.ll

45–80 ↗

(On Diff #346392)

Why the check lines are removed?

[anton-afanasyev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

3693–3695

There is no difference for now (the only one is compile time). We could also just prohibit marking NeedToGather for vector node (it could be done while checking for ephemeral values only).

But I've done it this way since it looks more transparent and also we could use zero cost for the future cases when/if such NeedToGather vector node is not the only member of tree and whole tree vectorization is beneficial.

4597–4599

Sure, fixed!

llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector-inseltpoison.ll

45–80 ↗

(On Diff #346392)

Fixed

[anton-afanasyev]

Address comments

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

3693–3695

I think it is better to use TTI::TCC_Expensive to mark it explicitly as an expensive operation. Or even invalid cost, if possible.

[anton-afanasyev]

Address comment

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

3693–3695

Ok, used Invalid cost.
Deleted gathering code, since gathering isn't supposed now.

[ABataev]

llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector-inseltpoison.ll

149–156 ↗

(On Diff #347373)

Why are the instructions reordered if no vectorization should happen?

[anton-afanasyev]

llvm/test/Transforms/SLPVectorizer/X86/insert-element-build-vector-inseltpoison.ll

149–156 ↗

(On Diff #347373)

Vectorization of inserts is prohibited now, yes, but compiler still "vectorizes" small trees of size 1 with any valid cost due to options -slp-threshold=-10000 -slp-min-tree-size=0. This is not related to this patch itself.

[ABataev]

LG

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

InstructionCost BoUpSLP::getEntryCost(const TreeEntry *E) {
   if (E->State == TreeEntry::NeedToGather) {
     if (allConstant(VL))
       return 0;
     if (isSplat(VL)) {
       return ReuseShuffleCost +
              TTI->getShuffleCost(TargetTransformInfo::SK_Broadcast, VecTy, None,
                                  0);
     }
+    // No cost for "gathering" already vector value
+    if (isa<FixedVectorType>(VL[0]->getType()))
+      return 0;
     if (E->getOpcode() == Instruction::ExtractElement &&
         allSameType(VL) && allSameBlock(VL)) {
       SmallVector<int> Mask;
       Optional<TargetTransformInfo::ShuffleKind> ShuffleKind =
           isShuffle(VL, Mask);
       if (ShuffleKind.hasValue()) {
         InstructionCost Cost =
             computeExtractCost(VL, VecTy, *ShuffleKind, Mask, *TTI);
void BoUpSLP::setInsertPointAfterBundle(const TreeEntry *E) {
   // debug location to Front.
   Builder.SetInsertPoint(BB, ++LastInst->getIterator());
   Builder.SetCurrentDebugLocation(Front->getDebugLoc());
 }
 
 Value *BoUpSLP::gather(ArrayRef<Value *> VL) {
   Value *Val0 =
       isa<StoreInst>(VL[0]) ? cast<StoreInst>(VL[0])->getValueOperand() : VL[0];
+  // "Gathering" of vector values is already done
+  if (isa<FixedVectorType>(Val0->getType()))
+    return Val0;
   FixedVectorType *VecTy = FixedVectorType::get(Val0->getType(), VL.size());
   Value *Vec = PoisonValue::get(VecTy);
   unsigned InsIndex = 0;
   for (Value *Val : VL) {
     Vec = Builder.CreateInsertElement(Vec, Val, Builder.getInt32(InsIndex++));
     auto *InsElt = dyn_cast<InsertElementInst>(Vec);
     if (!InsElt)
       continue;