This is an archive of the discontinued LLVM Phabricator instance.

Implement AVX ABI Warning/error
ClosedPublic

Authored by erichkeane on Jun 25 2020, 8:08 AM.

Details

Summary

The x86-64 "avx" feature changes how >128 bit vector types are passed,
instead of being passed in separate 128 bit registers, they can be
passed in 256 bit registers.

"avx512f" does the same thing, except it switches from 256 bit registers
to 512 bit registers.

The result of both of these is an ABI incompatibility between functions
compiled with and without these features.

This patch implements a warning/error pair upon an attempt to call a
function that would run afoul of this. First, if a function is called
that would have its ABI changed, we issue a warning.

Second, if said call is made in a situation where the caller and callee
are known to have different calling conventions (such as the case of
'target'), we instead issue an error.

Diff Detail

Event Timeline

erichkeane created this revision.Jun 25 2020, 8:08 AM
craig.topper added inline comments.Jun 28 2020, 4:03 PM
clang/include/clang/Basic/DiagnosticFrontendKinds.td
246

Should this be -Wpsabi to match gcc? I think that's what gcc called it.

clang/lib/CodeGen/CGCall.cpp
4248

Wow that was a big comment for no curly's.

4259

nit should probably be x86-64. I think we only use underscore when we're already using - for something else like in the triple.

clang/lib/CodeGen/TargetInfo.cpp
2493

Should these maps be const? Which means you have to use find to do the lookups.

2554

isArgument->IsArgument

2567

Same here

erichkeane marked 6 inline comments as done.

Do all feedback as @craig.topper recommended.

Instead of using 'find', I was able to use 'lookup' on the map, which seemed to be exactly what I want.

craig.topper added inline comments.Jun 29 2020, 6:12 PM
clang/lib/CodeGen/TargetInfo.cpp
2497

Should we save the lookup results in bool so we only do 2 lookups instead of 4 in the common case where there is no error/warning

erichkeane marked an inline comment as done.

As @craig.topper suggested, extract the four lookups into 2 bool instead.

craig.topper accepted this revision.Jun 30 2020, 8:08 PM

LGTM other than that one minor.

clang/lib/CodeGen/TargetInfo.cpp
2515

const the maps here too?

This revision is now accepted and ready to land.Jun 30 2020, 8:08 PM
This revision was automatically updated to reflect the committed changes.
erichkeane marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 7:33 AM
dmajor added a subscriber: dmajor.Aug 3 2020, 12:24 PM

@erichkeane, could you help me understand what is the action item of these warnings?

In Firefox we don't require AVX so our compilations generally don't enable the feature. (A very small number of files do come with AVX versions, mostly in imported media codecs, but they are reasonably well-isolated from other code.) It's not clear to me what the warning wants me to do. I can't enable AVX across the board, and I can't change all the code that uses large types as parameters. I feel like the only thing to do is silence the warning since there are enough instances that people will complain about log spam. Have I misunderstood the situation?

@erichkeane, could you help me understand what is the action item of these warnings?

In Firefox we don't require AVX so our compilations generally don't enable the feature. (A very small number of files do come with AVX versions, mostly in imported media codecs, but they are reasonably well-isolated from other code.) It's not clear to me what the warning wants me to do. I can't enable AVX across the board, and I can't change all the code that uses large types as parameters. I feel like the only thing to do is silence the warning since there are enough instances that people will complain about log spam. Have I misunderstood the situation?

With the warnings, yes, you can just disable them after you've made sure that you aren't compiling the functions with different settings. In the 'error' that this patch emits, it is that you have absolutely 'messed up'.

Basically:
If you try to call a function with a 256 bit type, and only 1 side uses 'avx' (either with 'target', multiversioning, or compiler flags), the ABI will not match. THIS is a programming mistake. If you're not making said programming mistake, suppressing the warning (with the suppression pragma) is your expected behavior.

jajadude added a subscriber: jajadude.EditedNov 1 2022, 3:28 AM
 if (Callee->getReturnType()->isVectorType() && CGM.getContext().getTypeSize(Callee->getReturnType()) > 128) {
}

I think this condition will make features like ext_vector_type be warnings too?

e.g. Apple's <simd/simd.h> provides
typedef __attribute__((__ext_vector_type__(3),__aligned__(16))) double simd_double3;

It's a vector of 3*64= 192 bits, larger than 128

clang/lib/CodeGen/TargetInfo.cpp
2566

I think this condition will make features like ext_vector_type to be warnings too?

e.g. Apple's <simd/simd.h> provides
typedef __attribute__((__ext_vector_type__(3),__aligned__(16))) double simd_double3;

It's a vector of 3*64= 192 bits, larger than 128

Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2022, 3:28 AM