[PATCH] D82562: Implement AVX ABI Warning/error

[Jake Li via Phabricator via cfe-commits]

jajadude added a comment.
Herald added subscribers: StephenFan, pengfei.
Herald added a project: All.

   if (Callee->getReturnType()->isVectorType() && 
CGM.getContext().getTypeSize(Callee->getReturnType()) > 128) {
  }

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

e.g. Apple's  provides
`typedef __attribute__((__ext_vector_type__(3),__aligned__(16))) double 
simd_double3;`

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




Comment at: clang/lib/CodeGen/TargetInfo.cpp:2566
+  if (Callee->getReturnType()->isVectorType() &&
+  CGM.getContext().getTypeSize(Callee->getReturnType()) > 128) {
+initFeatureMaps(CGM.getContext(), CallerMap, Caller, CalleeMap, Callee);

I think this condition will make features like 
[ext_vector_type](https://clang.llvm.org/docs/LanguageExtensions.html#vectors-and-extended-vectors)
 to be warnings too?

e.g.  Apple's `` provides
`typedef __attribute__((__ext_vector_type__(3),__aligned__(16))) double 
simd_double3;`

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82562: Implement AVX ABI Warning/error

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D82562#2191512 , @dmajor wrote:

> @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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82562: Implement AVX ABI Warning/error

[dmajor via Phabricator via cfe-commits]

dmajor added a comment.

@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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82562: Implement AVX ABI Warning/error

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
erichkeane marked an inline comment as done.
Closed by commit rG2831a317b689: Implement AVX ABI Warning/error (authored by 
erichkeane).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D82562?vs=274450&id=274800#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/target-avx-abi-diag.c
  clang/test/CodeGen/target-builtin-error-3.c
  clang/test/CodeGen/target-builtin-noerror.c

Index: clang/test/CodeGen/target-builtin-noerror.c
===
--- clang/test/CodeGen/target-builtin-noerror.c
+++ clang/test/CodeGen/target-builtin-noerror.c
@@ -6,15 +6,15 @@
 
 // No warnings.
 extern __m256i a;
-int __attribute__((target("avx"))) bar(__m256i a) {
+int __attribute__((target("avx"))) bar() {
   return _mm256_extract_epi32(a, 3);
 }
 
 int baz() {
-  return bar(a);
+  return bar();
 }
 
-int __attribute__((target("avx"))) qq_avx(__m256i a) {
+int __attribute__((target("avx"))) qq_avx() {
   return _mm256_extract_epi32(a, 3);
 }
 
@@ -25,7 +25,7 @@
 extern __m256i a;
 int qq() {
   if (__builtin_cpu_supports("avx"))
-return qq_avx(a);
+return qq_avx();
   else
 return qq_noavx();
 }
Index: clang/test/CodeGen/target-builtin-error-3.c
===
--- clang/test/CodeGen/target-builtin-error-3.c
+++ clang/test/CodeGen/target-builtin-error-3.c
@@ -18,11 +18,12 @@
   return __extension__ ({ __m256 __a = (a); (__m128i)__builtin_ia32_vcvtps2ph256((__v8sf)__a, (0x00)); }); // expected-error {{'__builtin_ia32_vcvtps2ph256' needs target feature f16c}}
 }
 static inline half16 __attribute__((__overloadable__)) convert_half( float16 a ) {
-  half16 r; 
-  r.lo = convert_half( a.lo); 
+  half16 r;
+  r.lo = convert_half(a.lo);
   return r;
 }
 void avx_test( uint16_t *destData, float16 argbF)
 {
-   ((half16U*)destData)[0] = convert_half(argbF);
+  // expected-warning@+1{{AVX vector argument of type 'float16' (vector of 16 'float' values) without 'avx512f' enabled changes the ABI}}
+  ((half16U *)destData)[0] = convert_half(argbF);
 }
Index: clang/test/CodeGen/target-avx-abi-diag.c
===
--- /dev/null
+++ clang/test/CodeGen/target-avx-abi-diag.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -verify=no256,no512 -o - -S
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -target-feature +avx -verify=no512 -o - -S
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -target-feature +avx512f -verify=both -o - -S
+
+// both-no-diagnostics
+
+typedef short avx512fType __attribute__((vector_size(64)));
+typedef short avx256Type __attribute__((vector_size(32)));
+
+__attribute__((target("avx"))) void takesAvx256(avx256Type t);
+__attribute__((target("avx512f"))) void takesAvx512(avx512fType t);
+void takesAvx256_no_target(avx256Type t);
+void takesAvx512_no_target(avx512fType t);
+
+void variadic(int i, ...);
+__attribute__((target("avx512f"))) void variadic_err(int i, ...);
+
+// If neither side has an attribute, warn.
+void call_warn(void) {
+  avx256Type t1;
+  takesAvx256_no_target(t1); // no256-warning {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+
+  avx512fType t2;
+  takesAvx512_no_target(t2); // no512-warning {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+
+  variadic(1, t1); // no256-warning {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  variadic(3, t2); // no512-warning {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+}
+
+// If only 1 side has an attribute, error.
+void call_errors(void) {
+  avx256Type t1;
+  takesAvx256(t1); // no256-error {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  avx512fType t2;
+  takesAvx512(t2); // no512-error {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+
+  variadic_err(1, t1); // no256-error {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  variadic_err(3, t2); // no512-error {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+}
+
+// These two don't diagnose anything, since these are valid calls.
+__attribute__((target("avx"))) void call_avx256_ok(void) {
+  avx256Type t;
+  takesAvx25

[PATCH] D82562: Implement AVX ABI Warning/error

[Craig Topper via Phabricator via cfe-commits]

craig.topper accepted this revision.
craig.topper added a comment.
This revision is now accepted and ready to land.

LGTM other than that one minor.




Comment at: clang/lib/CodeGen/TargetInfo.cpp:2515
+  SourceLocation CallLoc,
+  llvm::StringMap &CallerMap,
+  llvm::StringMap &CalleeMap, QualType Ty,

const the maps here too?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82562: Implement AVX ABI Warning/error

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 274450.
erichkeane marked an inline comment as done.
erichkeane added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/target-avx-abi-diag.c
  clang/test/CodeGen/target-builtin-error-3.c
  clang/test/CodeGen/target-builtin-noerror.c

Index: clang/test/CodeGen/target-builtin-noerror.c
===
--- clang/test/CodeGen/target-builtin-noerror.c
+++ clang/test/CodeGen/target-builtin-noerror.c
@@ -6,15 +6,15 @@
 
 // No warnings.
 extern __m256i a;
-int __attribute__((target("avx"))) bar(__m256i a) {
+int __attribute__((target("avx"))) bar() {
   return _mm256_extract_epi32(a, 3);
 }
 
 int baz() {
-  return bar(a);
+  return bar();
 }
 
-int __attribute__((target("avx"))) qq_avx(__m256i a) {
+int __attribute__((target("avx"))) qq_avx() {
   return _mm256_extract_epi32(a, 3);
 }
 
@@ -25,7 +25,7 @@
 extern __m256i a;
 int qq() {
   if (__builtin_cpu_supports("avx"))
-return qq_avx(a);
+return qq_avx();
   else
 return qq_noavx();
 }
Index: clang/test/CodeGen/target-builtin-error-3.c
===
--- clang/test/CodeGen/target-builtin-error-3.c
+++ clang/test/CodeGen/target-builtin-error-3.c
@@ -18,11 +18,12 @@
   return __extension__ ({ __m256 __a = (a); (__m128i)__builtin_ia32_vcvtps2ph256((__v8sf)__a, (0x00)); }); // expected-error {{'__builtin_ia32_vcvtps2ph256' needs target feature f16c}}
 }
 static inline half16 __attribute__((__overloadable__)) convert_half( float16 a ) {
-  half16 r; 
-  r.lo = convert_half( a.lo); 
+  half16 r;
+  r.lo = convert_half(a.lo);
   return r;
 }
 void avx_test( uint16_t *destData, float16 argbF)
 {
-   ((half16U*)destData)[0] = convert_half(argbF);
+  // expected-warning@+1{{AVX vector argument of type 'float16' (vector of 16 'float' values) without 'avx512f' enabled changes the ABI}}
+  ((half16U *)destData)[0] = convert_half(argbF);
 }
Index: clang/test/CodeGen/target-avx-abi-diag.c
===
--- /dev/null
+++ clang/test/CodeGen/target-avx-abi-diag.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -verify=no256,no512 -o - -S
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -target-feature +avx -verify=no512 -o - -S
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -target-feature +avx512f -verify=both -o - -S
+
+// both-no-diagnostics
+
+typedef short avx512fType __attribute__((vector_size(64)));
+typedef short avx256Type __attribute__((vector_size(32)));
+
+__attribute__((target("avx"))) void takesAvx256(avx256Type t);
+__attribute__((target("avx512f"))) void takesAvx512(avx512fType t);
+void takesAvx256_no_target(avx256Type t);
+void takesAvx512_no_target(avx512fType t);
+
+void variadic(int i, ...);
+__attribute__((target("avx512f"))) void variadic_err(int i, ...);
+
+// If neither side has an attribute, warn.
+void call_warn(void) {
+  avx256Type t1;
+  takesAvx256_no_target(t1); // no256-warning {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+
+  avx512fType t2;
+  takesAvx512_no_target(t2); // no512-warning {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+
+  variadic(1, t1); // no256-warning {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  variadic(3, t2); // no512-warning {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+}
+
+// If only 1 side has an attribute, error.
+void call_errors(void) {
+  avx256Type t1;
+  takesAvx256(t1); // no256-error {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  avx512fType t2;
+  takesAvx512(t2); // no512-error {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+
+  variadic_err(1, t1); // no256-error {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  variadic_err(3, t2); // no512-error {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+}
+
+// These two don't diagnose anything, since these are valid calls.
+__attribute__((target("avx"))) void call_avx256_ok(void) {
+  avx256Type t;
+  takesAvx256(t);
+}
+
+__attribute__((target("avx512f"))) void call_avx512_ok(void) {
+  avx512fType t;
+  takesAvx512(t);
+}
Index: clang/lib/CodeGen/TargetInfo.h
==

[PATCH] D82562: Implement AVX ABI Warning/error

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2497
+ bool IsArgument) {
+  if (!CallerMap.lookup(Feature) && !CalleeMap.lookup(Feature))
+return Diag.Report(CallLoc, diag::warn_avx_calling_convention)

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82562: Implement AVX ABI Warning/error

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 274106.
erichkeane marked 6 inline comments as done.
erichkeane added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/target-avx-abi-diag.c
  clang/test/CodeGen/target-builtin-error-3.c
  clang/test/CodeGen/target-builtin-noerror.c

Index: clang/test/CodeGen/target-builtin-noerror.c
===
--- clang/test/CodeGen/target-builtin-noerror.c
+++ clang/test/CodeGen/target-builtin-noerror.c
@@ -6,15 +6,15 @@
 
 // No warnings.
 extern __m256i a;
-int __attribute__((target("avx"))) bar(__m256i a) {
+int __attribute__((target("avx"))) bar() {
   return _mm256_extract_epi32(a, 3);
 }
 
 int baz() {
-  return bar(a);
+  return bar();
 }
 
-int __attribute__((target("avx"))) qq_avx(__m256i a) {
+int __attribute__((target("avx"))) qq_avx() {
   return _mm256_extract_epi32(a, 3);
 }
 
@@ -25,7 +25,7 @@
 extern __m256i a;
 int qq() {
   if (__builtin_cpu_supports("avx"))
-return qq_avx(a);
+return qq_avx();
   else
 return qq_noavx();
 }
Index: clang/test/CodeGen/target-builtin-error-3.c
===
--- clang/test/CodeGen/target-builtin-error-3.c
+++ clang/test/CodeGen/target-builtin-error-3.c
@@ -18,11 +18,12 @@
   return __extension__ ({ __m256 __a = (a); (__m128i)__builtin_ia32_vcvtps2ph256((__v8sf)__a, (0x00)); }); // expected-error {{'__builtin_ia32_vcvtps2ph256' needs target feature f16c}}
 }
 static inline half16 __attribute__((__overloadable__)) convert_half( float16 a ) {
-  half16 r; 
-  r.lo = convert_half( a.lo); 
+  half16 r;
+  r.lo = convert_half(a.lo);
   return r;
 }
 void avx_test( uint16_t *destData, float16 argbF)
 {
-   ((half16U*)destData)[0] = convert_half(argbF);
+  // expected-warning@+1{{AVX vector argument of type 'float16' (vector of 16 'float' values) without 'avx512f' enabled changes the ABI}}
+  ((half16U *)destData)[0] = convert_half(argbF);
 }
Index: clang/test/CodeGen/target-avx-abi-diag.c
===
--- /dev/null
+++ clang/test/CodeGen/target-avx-abi-diag.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -verify=no256,no512 -o - -S
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -target-feature +avx -verify=no512 -o - -S
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -target-feature +avx512f -verify=both -o - -S
+
+// both-no-diagnostics
+
+typedef short avx512fType __attribute__((vector_size(64)));
+typedef short avx256Type __attribute__((vector_size(32)));
+
+__attribute__((target("avx"))) void takesAvx256(avx256Type t);
+__attribute__((target("avx512f"))) void takesAvx512(avx512fType t);
+void takesAvx256_no_target(avx256Type t);
+void takesAvx512_no_target(avx512fType t);
+
+void variadic(int i, ...);
+__attribute__((target("avx512f"))) void variadic_err(int i, ...);
+
+// If neither side has an attribute, warn.
+void call_warn(void) {
+  avx256Type t1;
+  takesAvx256_no_target(t1); // no256-warning {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+
+  avx512fType t2;
+  takesAvx512_no_target(t2); // no512-warning {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+
+  variadic(1, t1); // no256-warning {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  variadic(3, t2); // no512-warning {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+}
+
+// If only 1 side has an attribute, error.
+void call_errors(void) {
+  avx256Type t1;
+  takesAvx256(t1); // no256-error {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  avx512fType t2;
+  takesAvx512(t2); // no512-error {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+
+  variadic_err(1, t1); // no256-error {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  variadic_err(3, t2); // no512-error {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+}
+
+// These two don't diagnose anything, since these are valid calls.
+__attribute__((target("avx"))) void call_avx256_ok(void) {
+  avx256Type t;
+  takesAvx256(t);
+}
+
+__attribute__((target("avx512f"))) void call_avx512_ok(void) {
+  avx512fType

[PATCH] D82562: Implement AVX ABI Warning/error

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:246
+  "enabled changes the ABI">,
+  InGroup>;
+def err_avx_calling_convention : Error;

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



Comment at: clang/lib/CodeGen/CGCall.cpp:4248
   const Decl *TargetDecl = Callee.getAbstractInfo().getCalleeDecl().getDecl();
-  if (const FunctionDecl *FD = dyn_cast_or_null(TargetDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(TargetDecl)) {
 // We can only guarantee that a function is called from the correct

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



Comment at: clang/lib/CodeGen/CGCall.cpp:4259
 
+// Some architectures (such as x86_64) have the ABI changed based on
+// attribute-target/features. Give them a chance to diagnose.

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



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2493
+ SourceLocation CallLoc,
+ llvm::StringMap &CallerMap,
+ llvm::StringMap &CalleeMap, QualType Ty,

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



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2554
+  if (checkAVXParam(CGM.getDiags(), CGM.getContext(), CallLoc, CallerMap,
+CalleeMap, Ty, /*isArgument*/ true))
+return;

isArgument->IsArgument



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2567
+  CalleeMap, Callee->getReturnType(),
+  /*isArgument*/ false);
+  }

Same here


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82562/new/

https://reviews.llvm.org/D82562



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82562: Implement AVX ABI Warning/error

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added reviewers: craig.topper, echristo.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D82562

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/target-avx-abi-diag.c
  clang/test/CodeGen/target-builtin-error-3.c
  clang/test/CodeGen/target-builtin-noerror.c

Index: clang/test/CodeGen/target-builtin-noerror.c
===
--- clang/test/CodeGen/target-builtin-noerror.c
+++ clang/test/CodeGen/target-builtin-noerror.c
@@ -6,15 +6,15 @@
 
 // No warnings.
 extern __m256i a;
-int __attribute__((target("avx"))) bar(__m256i a) {
+int __attribute__((target("avx"))) bar() {
   return _mm256_extract_epi32(a, 3);
 }
 
 int baz() {
-  return bar(a);
+  return bar();
 }
 
-int __attribute__((target("avx"))) qq_avx(__m256i a) {
+int __attribute__((target("avx"))) qq_avx() {
   return _mm256_extract_epi32(a, 3);
 }
 
@@ -25,7 +25,7 @@
 extern __m256i a;
 int qq() {
   if (__builtin_cpu_supports("avx"))
-return qq_avx(a);
+return qq_avx();
   else
 return qq_noavx();
 }
Index: clang/test/CodeGen/target-builtin-error-3.c
===
--- clang/test/CodeGen/target-builtin-error-3.c
+++ clang/test/CodeGen/target-builtin-error-3.c
@@ -18,11 +18,12 @@
   return __extension__ ({ __m256 __a = (a); (__m128i)__builtin_ia32_vcvtps2ph256((__v8sf)__a, (0x00)); }); // expected-error {{'__builtin_ia32_vcvtps2ph256' needs target feature f16c}}
 }
 static inline half16 __attribute__((__overloadable__)) convert_half( float16 a ) {
-  half16 r; 
-  r.lo = convert_half( a.lo); 
+  half16 r;
+  r.lo = convert_half(a.lo);
   return r;
 }
 void avx_test( uint16_t *destData, float16 argbF)
 {
-   ((half16U*)destData)[0] = convert_half(argbF);
+  // expected-warning@+1{{AVX vector argument of type 'float16' (vector of 16 'float' values) without 'avx512f' enabled changes the ABI}}
+  ((half16U *)destData)[0] = convert_half(argbF);
 }
Index: clang/test/CodeGen/target-avx-abi-diag.c
===
--- /dev/null
+++ clang/test/CodeGen/target-avx-abi-diag.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -verify=no256,no512 -o - -S
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -target-feature +avx -verify=no512 -o - -S
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -target-feature +avx512f -verify=both -o - -S
+
+// both-no-diagnostics
+
+typedef short avx512fType __attribute__((vector_size(64)));
+typedef short avx256Type __attribute__((vector_size(32)));
+
+__attribute__((target("avx"))) void takesAvx256(avx256Type t);
+__attribute__((target("avx512f"))) void takesAvx512(avx512fType t);
+void takesAvx256_no_target(avx256Type t);
+void takesAvx512_no_target(avx512fType t);
+
+void variadic(int i, ...);
+__attribute__((target("avx512f"))) void variadic_err(int i, ...);
+
+// If neither side has an attribute, warn.
+void call_warn(void) {
+  avx256Type t1;
+  takesAvx256_no_target(t1); // no256-warning {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+
+  avx512fType t2;
+  takesAvx512_no_target(t2); // no512-warning {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+
+  variadic(1, t1); // no256-warning {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  variadic(3, t2); // no512-warning {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+}
+
+// If only 1 side has an attribute, error.
+void call_errors(void) {
+  avx256Type t1;
+  takesAvx256(t1); // no256-error {{AVX vector argument of type 'avx256Type' (vector of 16 'short' values) without 'avx' enabled changes the ABI}}
+  avx512fType t2;
+  takesAvx512(t2); // no512-error {{AVX vector argument of type 'avx512fType' (vector of 32 'short' values) without 'avx512f' enabled changes the ABI}}
+
+  variadic_err(1, t1); // no256-error {