Page MenuHomePhabricator

[X86] Prevent passing vectors of __int128 as <X x i128> in llvm IR
ClosedPublic

Authored by craig.topper on Jul 12 2019, 3:08 PM.

Details

Summary

As far as I can tell, gcc passes 256/512 bit vectors __int128 in memory. And passes a vector of 1 _int128 in an xmm register. The backend considers <X x i128> as an illegal type and will scalarize any arguments with that type. So we need to coerce the argument types in the frontend to match to avoid the illegal type.

Are there other element types to consider? Do we need to keep the old behavior on platforms where clang is the de facto compiler?

This issue was identified in PR42607. Though even with the types changed, we still seem to be doing some unnecessary stack realignment.

I'll add test cases later today or over the weekend.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 12 2019, 3:08 PM
RKSimon edited reviewers, added: efriedma, gbedwell, wristow; removed: eli.friedman.Jul 13 2019, 12:46 AM

Do we need to keep the old behavior on platforms where clang is the de facto compiler?

I know we (PlayStation) will want to keep the old behavior.

Maybe this ABI issue should be fixed for 9.0 ? Not sure how important, but
@hans maybe should track this one.

craig.topper edited the summary of this revision. (Show Details)

Add test cases. Change the type to vXi64 instead of vXf64. Add abi compatibility flag. Restrict to linux and freebsd.

Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2019, 12:51 PM

More test check lines. Test the compatibility flag.

hans added a comment.Aug 13 2019, 4:26 AM

Maybe this ABI issue should be fixed for 9.0 ? Not sure how important, but
@hans maybe should track this one.

IIUC, this is not a new issue, so I'd prefer not trying to rush it in for llvm 9, and rather get it right for llvm 10 instead.

Please can you extend the test coverage to cover more vector sizes and cpu target features, not just avx512, and also add x86_64-scei-ps4 triple tests. Maybe 32-bit tests as well?

clang/include/clang/Basic/LangOptions.h
143

Add that this is just for Linux and NetBSD

Please can you extend the test coverage to cover more vector sizes and cpu target features, not just avx512, and also add x86_64-scei-ps4 triple tests. Maybe 32-bit tests as well?

Other than 256-bit what other size do you want?

Add more tests. Clarify which platforms are affected in LangOptions.h

Tests look great - please can you pre-commit them and update the patch to show the diff? Also, maybe call the test file x86-vec-i128.c and add a comment in the file describing PR42607?

Tests look great - please can you pre-commit them and update the patch to show the diff? Also, maybe call the test file x86-vec-i128.c and add a comment in the file describing PR42607?

How do you want me to show the diff? There a couple options. Three of the check lines have to be removed from the pre-commit since they use a command line option that doesn't exist. Do you want me to change the check lines that say NEWABI to use the old code? Or do you want me to manipulate the FileCheck command lines to use the OLDABI check lines?

Tests look great - please can you pre-commit them and update the patch to show the diff? Also, maybe call the test file x86-vec-i128.c and add a comment in the file describing PR42607?

How do you want me to show the diff? There a couple options. Three of the check lines have to be removed from the pre-commit since they use a command line option that doesn't exist. Do you want me to change the check lines that say NEWABI to use the old code? Or do you want me to manipulate the FileCheck command lines to use the OLDABI check lines?

How about CLANG9ABI and CLANG10ABI? And the patch just adds the -fclang-abi-compat=9 tests?

Change test name and adjust to the changes I think @RKSimon was asking for. I haven't commited the test yet. I'd like to verify that the before version of the test is how you would like it pre-committed

It looks to me like the patch does things the New Way only for Linux and NetBSD, so for PS4 backward compatibility I am okay with it.

RKSimon accepted this revision.Wed, Sep 4, 7:23 AM

LGTM - cheers

This revision is now accepted and ready to land.Wed, Sep 4, 7:23 AM

@craig.topper Wasn't this committed at rL371169 ?

craig.topper closed this revision.Fri, Sep 6, 10:28 AM

Committed in r371169, but I forgot the Differential Revision line