This is an archive of the discontinued LLVM Phabricator instance.

Fix the trunc instruction insertion problem in SLP pass
ClosedPublic

Authored by bule on Mar 11 2021, 6:43 AM.

Details

Summary

Current SLP pass has this piece of code that inserts a trunc instruction after the vectorized instruction. In the case that the vectorized instruction is a phi node and not the last phi node in the BB, the trunc instruction will be inserted between two phi nodes, which will trigger verify problem in debug version or unpredictable error in another pass. See the testcase in the patch as an example. Try remove the -disable-verify option for the verify error.

This patch changes the algorithm to 'if the last vectorized instruction is a phi, insert it after the last phi node in current BB' to fix this problem.

Diff Detail

Event Timeline

bule created this revision.Mar 11 2021, 6:43 AM
bule requested review of this revision.Mar 11 2021, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 6:43 AM

Please can you regenerate the diff with context (git diff -U9999) ?

Also could you please precommit test or just make diff against it (to see test changes before/after patch)?

Also could you please precommit test or just make diff against it (to see test changes before/after patch)?

AFAICT the test currently crashes

Also could you please precommit test or just make diff against it (to see test changes before/after patch)?

AFAICT the test currently crashes

https://reviews.llvm.org/D98596 , pre-commit patch added

bule updated this revision to Diff 330492.Mar 13 2021, 7:41 PM

add -U9999 diff patch and the change after the pre-commit patch

bule added a comment.Mar 13 2021, 7:43 PM

Please can you regenerate the diff with context (git diff -U9999) ?

regenerated with pre-commit change. Updated.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4915

isa<PHINode>(I)

bjope added a subscriber: bjope.Mar 14 2021, 5:28 AM
bjope added inline comments.
llvm/test/Transforms/SLPVectorizer/AArch64/trunc-insertion.ll
1–2

I figure the idea is that -disable-verify can be removed in this patch.

bule updated this revision to Diff 330513.Mar 14 2021, 8:58 AM

Use the isa function to check the phi node and remove the -disable-verify option in the testcase.

This revision is now accepted and ready to land.Mar 14 2021, 10:41 AM
bule updated this revision to Diff 331006.Mar 16 2021, 9:08 AM

The previous testcase didn't pass the pre-merge check because an element is duplicated. The precommit test case have been changed. This patch fix the problem along with the pre-commit patch.

bule added a comment.Mar 17 2021, 2:29 AM

@anton-afanasyev Anton, you might need to help me merge this one as well.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4915

done

llvm/test/Transforms/SLPVectorizer/AArch64/trunc-insertion.ll
1–2

@anton-afanasyev what do you think?

1–2

Remove the -disable-verify option could make the result check more strict. So removed.

llvm/test/Transforms/SLPVectorizer/AArch64/trunc-insertion.ll
1–2

Looks good.

Closed by commit: https://reviews.llvm.org/rG9abe50047330
(modified summary a bit to comply with changes)