[SLPVectorizer] Add shuffle instruction cost for jumbled load

[• ashahid]

Summary

This fixes the missing cost accounting for extra cost of shuffle instruction due to the
vectorization of jumbled load added through the patch https://reviews.llvm.org/D36130.

[• ashahid]

Regression and LNT test was successful. Bootstrap test is underway.

[• ashahid]

Ping!

[eastig]

This comment has been deleted.

lib/Transforms/Vectorize/SLPVectorizer.cpp

2229

Maybe to move this into the if-statement:

if (TreeEntry *UserTreeEntry = &VectorizableTree[Idx])

2234

I am not an expert in SLPVectorizer.
I see there can be different ShuffleKind. Why is it always SK_PermuteSingleSrc?
I also see there is the case for Instruction::ShuffleVector. Why is this shuffle cost not calculated there? Won't we have double cost calculation?

[eastig]

In D41324#957637, @ashahid wrote:

Regression and LNT test was successful. Bootstrap test is underway.

Hi Shahid,

Do you mean the LNT test code is back to the code before the patch D36130?

Thanks,
Evgeny

[• ashahid]

In D41324#958204, @eastig wrote:

In D41324#957637, @ashahid wrote:

Regression and LNT test was successful. Bootstrap test is underway.

Hi Shahid,

Do you mean the LNT test code is back to the code before the patch D36130?

Thanks,
Evgeny

Sorry, I could not get your question.
Recently I started running these lit test, LNT test and bootstrap test to make sure build bot does not break upon commit.

Regards,
Shahid

lib/Transforms/Vectorize/SLPVectorizer.cpp

2229

Sure.

2234

Since this shuffle instruction is only introduced to reorder the loaded lanes which is jumbled, it falls under SK_PermuteSingleSrc shuffle kind. Also due to this very reason
it is only required to be considered under LOAD case but not in other cases.

[• ashahid]

lib/Transforms/Vectorize/SLPVectorizer.cpp

2234

Also, since we are not creating new tree entry for this shuffle, we will not do double cost calculation.

[eastig]

In D41324#958398, @ashahid wrote:

In D41324#958204, @eastig wrote:

In D41324#957637, @ashahid wrote:

Regression and LNT test was successful. Bootstrap test is underway.

Hi Shahid,

Do you mean the LNT test code is back to the code before the patch D36130?

Thanks,
Evgeny

Sorry, I could not get your question.
Recently I started running these lit test, LNT test and bootstrap test to make sure build bot does not break upon commit.

Regards,
Shahid

Now I see what you meant. It was not clear for me whether you checked the execution time of the regressed benchmarks or not. It is not a problem. I can check it using the patch.
I don't have any other questions.

[• ashahid]

Review comment updated accordingly.

[• ashahid]

Lit test done, LNT test underway.

[Ayal]

Presumably this fixes the reported regressions?

Cf. the test added in D40883 to check if a cost is accounted for (or not).

lib/Transforms/Vectorize/SLPVectorizer.cpp

2229

Surely &VectorizableTree[Idx] is not null.

This also applies btw to checking it newTreeEntry().

2230

Have Mask be a reference, to a const SmallVector?

[• ashahid]

In D41324#959193, @Ayal wrote:

Presumably this fixes the reported regressions?

Cf. the test added in D40883 to check if a cost is accounted for (or not).

This test was meant for Loop Vectorizer, do you want me to check this for SLP or I may be missing something.

lib/Transforms/Vectorize/SLPVectorizer.cpp

2229

Ah, that's right.

2230

Sure.

[xbolva00]

Test?

lib/Transforms/Vectorize/SLPVectorizer.cpp

switch (ShuffleOrOp) {
     }
     case Instruction::Load: {
       // Cost of wide load - cost of scalar loads.
       unsigned alignment = dyn_cast<LoadInst>(VL0)->getAlignment();
       int ScalarLdCost = VecTy->getNumElements() *
           TTI->getMemoryOpCost(Instruction::Load, ScalarTy, alignment, 0, VL0);
       int VecLdCost = TTI->getMemoryOpCost(Instruction::Load,
                                            VecTy, alignment, 0, VL0);
+      // Add the cost of *shuffle* for jumbled loads
+      for (unsigned Idx : E->UserTreeIndices) {
+        if (TreeEntry *UserTreeEntry = &VectorizableTree[Idx]) {
+          for (SmallVector<unsigned, 4> Mask : UserTreeEntry->ShuffleMask) {
+            if (!Mask.empty()) {
+              VecLdCost += TTI->getShuffleCost(
+                  TargetTransformInfo::SK_PermuteSingleSrc, VecTy, 0);
+            }
+          }
+        }
+      }
       return VecLdCost - ScalarLdCost;
     }
     case Instruction::Store: {
       // We know that we can merge the stores. Calculate the cost.
       unsigned alignment = dyn_cast<StoreInst>(VL0)->getAlignment();
       int ScalarStCost = VecTy->getNumElements() *
           TTI->getMemoryOpCost(Instruction::Store, ScalarTy, alignment, 0, VL0);
       int VecStCost = TTI->getMemoryOpCost(Instruction::Store,