Page MenuHomePhabricator

Don't widen shuffle element with AVX512
Needs ReviewPublic

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

Details

Reviewers
RKSimon
pengfei
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

Unit TestsFailed

TimeTest
60,120 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,100 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp

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
16872

Fix crashing.

17633

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
16872

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 ?

17633

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
16860

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
16860

I think this should be OK

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