This is an archive of the discontinued LLVM Phabricator instance.

Don't widen shuffle element with AVX512
ClosedPublic

Authored by LuoYuanke on Jul 30 2022, 8:42 PM.

Details

Summary

Fix crash issue of D129537 and reopen it.

Currently the X86 shuffle lowering would widen the element type for
shuffle if the mask element value is adjacent. For below example

%t2 = add nsw <16 x i32> %t0, %t1
%t3 = sub nsw <16 x i32> %t0, %t1
%t4 = shufflevector <16 x i32> %t2, <16 x i32> %t3,
                    <16 x i32> <i32 16, i32 17, i32 2, i32 3, i32 4,
                     i32 5, i32 6, i32 7, i32 8, i32 9, i32 10,
                     i32 11, i32 12, i32 13, i32 14, i32 15>

ret <16 x i32> %t4

Compiler would transform the shuffle to

%t4 = shufflevector <8 x i64> %t2, <8 x i64> %t3,
                    <8 x i64> <i32 8, i32 1, i32 2, i32 3, i32 4,
                               i32 5, i32 6, i32 7>

This may lose the oppotunity to let ISel select mask instruction when
avx512 is enabled.

This patch is to prevent the tranform when avx512 feature is enabled.
Thank Simon for the idea.

Diff Detail

Event Timeline

LuoYuanke created this revision.Jul 30 2022, 8:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2022, 8:42 PM
LuoYuanke requested review of this revision.Jul 30 2022, 8:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2022, 8:42 PM
LuoYuanke added inline comments.Jul 30 2022, 8:45 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
16890

Fix crashing.

17657

Fix crashing.

llvm/test/CodeGen/X86/avx512-shuffles/shuffle-blend.ll
252

This case cause crashing previously.

264

This case cause crashing previously.

RKSimon added inline comments.Aug 2 2022, 5:19 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
16890

We really need to avoid creating nodes if we're going to bail - can this be merged into the CanUseSublanes check above? Or just a relaxed comparison of Mask and InLaneMask ?

17657

Again - don't create a shuffle if we're not going to use it.

Also - please can you raise a 15.x bug - I guess this will need merging into the release branch, which probably didn't merge the revert either?

LuoYuanke added a comment.EditedAug 2 2022, 7:52 PM

Also - please can you raise a 15.x bug - I guess this will need merging into the release branch, which probably didn't merge the revert either?

Thank Simon for reminding. I created D131042 to cherry pick the fix to 15.x.

Created https://github.com/llvm/llvm-project/issues/56892.

LuoYuanke updated this revision to Diff 451444.Aug 10 2022, 6:56 AM

Address Simon's comments.

LuoYuanke added inline comments.Aug 10 2022, 7:06 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
16877

If CrossLaneMask == Mask, InLaneMask must be identity mask. However I'm not sure if InLaneMask == Mask CrossLaneMask must be identity mask.

Sorry - I lost track of this patch - have we committed test coverage for the regression mentioned on D129537?

llvm/lib/Target/X86/X86ISelLowering.cpp
16877

I think this should be OK

Matt added a subscriber: Matt.Sep 27 2022, 11:44 AM

Sorry - I lost track of this patch - have we committed test coverage for the regression mentioned on D129537?

Yes, the test case has been committed at revision 6b4c386b1e7060d.

@LuoYuanke reverse-ping

@RKSimon, I think I am waiting for your approve.

RKSimon accepted this revision.Oct 12 2022, 3:26 AM

LGTM with a couple of minors

llvm/lib/Target/X86/X86ISelLowering.cpp
16885

revert this NFC - use the original return DAG.getVectorShuffle

17661

return DAG.getVectorShuffle

This revision is now accepted and ready to land.Oct 12 2022, 3:26 AM
LuoYuanke updated this revision to Diff 467081.Oct 12 2022, 4:10 AM

Address Simon's comments and rebase.

This revision was landed with ongoing or failed builds.Oct 12 2022, 4:23 AM
This revision was automatically updated to reflect the committed changes.