Page MenuHomePhabricator

[SLPVectorizer] Add shuffle instruction cost for jumbled load
Needs ReviewPublic

Authored by ashahid on Dec 16 2017, 9:22 AM.

Details

Reviewers
Ayal
eastig
mkuper
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.

Event Timeline

ashahid created this revision.Dec 16 2017, 9:22 AM
ashahid edited the summary of this revision. (Show Details)Dec 16 2017, 9:25 AM
ashahid added reviewers: Ayal, eastig, mkuper.
ashahid edited the summary of this revision. (Show Details)Dec 16 2017, 9:27 AM

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

eastig added a comment.EditedDec 18 2017, 3:49 AM
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?

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

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 added inline comments.Dec 18 2017, 8:03 AM
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.

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.

Review comment updated accordingly.

Lit test done, LNT test underway.

Ayal added a comment.Dec 19 2017, 2:44 AM

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?

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.