public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
@ 2021-11-09 10:09 apinski
  2021-11-09 14:38 ` Richard Sandiford
  2022-12-07  2:21 ` Kewen.Lin
  0 siblings, 2 replies; 9+ messages in thread
From: apinski @ 2021-11-09 10:09 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andrew Pinski

From: Andrew Pinski <apinski@marvell.com>

This fixes fully where SVE types were being used without sve being enabled.
Instead of trying to fix it such that we error out during RTL time, it is
better to error out in front-ends.  This expands verify_type_context to
have a context of auto storage decl which is used for both auto storage
decls and for indirection context.

A few testcases needed to be updated for the new error message; they were
already being rejected before hand.

OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.

PR target/99657
gcc/c/ChangeLog:

	* c-decl.c (finish_decl): Call verify_type_context
	for all decls and not just global_decls.
	* c-typeck.c (build_indirect_ref): Call verify_type_context
	to check to see if the type is ok to be used.

gcc/ChangeLog:

	* config/aarch64/aarch64-sve-builtins.cc (verify_type_context):
	Add TXTC_AUTO_STORAGE support
	* target.h (enum type_context_kind): Add TXTC_AUTO_STORAGE.

gcc/cp/ChangeLog:

	* decl.c (cp_finish_decl): Call verify_type_context
	for all decls and not just global_decls.
	* typeck.c (cp_build_indirect_ref_1): Call verify_type_context
	to check to see if the type is ok to be used.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/sve/acle/general/nosve_1.c: Update test.
	* gcc.target/aarch64/sve/acle/general/nosve_4.c: Likewise.
	* gcc.target/aarch64/sve/acle/general/nosve_5.c: Likewise.
	* gcc.target/aarch64/sve/acle/general/nosve_6.c: Likewise.
	* gcc.target/aarch64/sve/pcs/nosve_2.c: Likewise.
	* gcc.target/aarch64/sve/pcs/nosve_3.c: Likewise.
	* gcc.target/aarch64/sve/pcs/nosve_4.c: Likewise.
	* gcc.target/aarch64/sve/pcs/nosve_5.c: Likewise.
	* gcc.target/aarch64/sve/pcs/nosve_6.c: Likewise.
	* gcc.target/aarch64/sve/pcs/nosve_9.c: New test.
---
 gcc/c/c-decl.c                                    | 14 +++++++-------
 gcc/c/c-typeck.c                                  |  2 ++
 gcc/config/aarch64/aarch64-sve-builtins.cc        | 14 ++++++++++++++
 gcc/cp/decl.c                                     | 10 ++++++----
 gcc/cp/typeck.c                                   |  4 ++++
 gcc/target.h                                      |  3 +++
 .../gcc.target/aarch64/sve/acle/general/nosve_1.c |  1 +
 .../gcc.target/aarch64/sve/acle/general/nosve_4.c |  2 +-
 .../gcc.target/aarch64/sve/acle/general/nosve_5.c |  2 +-
 .../gcc.target/aarch64/sve/acle/general/nosve_6.c |  1 +
 .../gcc.target/aarch64/sve/pcs/nosve_2.c          |  2 +-
 .../gcc.target/aarch64/sve/pcs/nosve_3.c          |  2 +-
 .../gcc.target/aarch64/sve/pcs/nosve_4.c          |  3 +--
 .../gcc.target/aarch64/sve/pcs/nosve_5.c          |  3 +--
 .../gcc.target/aarch64/sve/pcs/nosve_6.c          |  3 +--
 .../gcc.target/aarch64/sve/pcs/nosve_9.c          | 15 +++++++++++++++
 16 files changed, 60 insertions(+), 21 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c

diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 186fa1692c1..b3583622475 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -5441,19 +5441,19 @@ finish_decl (tree decl, location_t init_loc, tree init,
 
   if (VAR_P (decl))
     {
+      type_context_kind context = TCTX_AUTO_STORAGE;
       if (init && TREE_CODE (init) == CONSTRUCTOR)
 	add_flexible_array_elts_to_size (decl, init);
 
       complete_flexible_array_elts (DECL_INITIAL (decl));
 
       if (is_global_var (decl))
-	{
-	  type_context_kind context = (DECL_THREAD_LOCAL_P (decl)
-				       ? TCTX_THREAD_STORAGE
-				       : TCTX_STATIC_STORAGE);
-	  if (!verify_type_context (input_location, context, TREE_TYPE (decl)))
-	    TREE_TYPE (decl) = error_mark_node;
-	}
+	context = (DECL_THREAD_LOCAL_P (decl)
+		   ? TCTX_THREAD_STORAGE
+		   : TCTX_STATIC_STORAGE);
+
+      if (!verify_type_context (input_location, context, TREE_TYPE (decl)))
+	TREE_TYPE (decl) = error_mark_node;
 
       if (DECL_SIZE (decl) == NULL_TREE && TREE_TYPE (decl) != error_mark_node
 	  && COMPLETE_TYPE_P (TREE_TYPE (decl)))
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 782414f8c8c..e926b7c1964 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -2630,6 +2630,8 @@ build_indirect_ref (location_t loc, tree ptr, ref_operator errstring)
       else
 	{
 	  tree t = TREE_TYPE (type);
+	  if (!verify_type_context (loc, TCTX_AUTO_STORAGE, t))
+	    return error_mark_node;
 
 	  ref = build1 (INDIRECT_REF, t, pointer);
 
diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc b/gcc/config/aarch64/aarch64-sve-builtins.cc
index bc92213665c..1d5083bf9fa 100644
--- a/gcc/config/aarch64/aarch64-sve-builtins.cc
+++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
@@ -3834,11 +3834,25 @@ bool
 verify_type_context (location_t loc, type_context_kind context,
 		     const_tree type, bool silent_p)
 {
+  static bool informed = false;
   if (!sizeless_type_p (type))
     return true;
 
   switch (context)
     {
+    case TCTX_AUTO_STORAGE:
+      if (TARGET_SVE)
+	return true;
+      if (!silent_p && !informed)
+	{
+	  informed = true;
+	  error_at (loc, "SVE type %qT used without SVE ISA enabled", type);
+	  inform (loc, "you can enable SVE using the command-line"
+		  " option %<-march%>, or by using the %<target%>"
+		  " attribute or pragma");
+	}
+      return false;
+
     case TCTX_SIZEOF:
     case TCTX_STATIC_STORAGE:
       if (!silent_p)
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 947bbfc6637..1ffd11acae0 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -7975,11 +7975,13 @@ cp_finish_decl (tree decl, tree init, bool init_const_expr_p,
       && DECL_INITIALIZED_IN_CLASS_P (decl))
     check_static_variable_definition (decl, type);
 
-  if (!processing_template_decl && VAR_P (decl) && is_global_var (decl))
+  if (!processing_template_decl && VAR_P (decl))
     {
-      type_context_kind context = (DECL_THREAD_LOCAL_P (decl)
-				   ? TCTX_THREAD_STORAGE
-				   : TCTX_STATIC_STORAGE);
+      type_context_kind context = TCTX_AUTO_STORAGE;
+      if (is_global_var (decl))
+	context = (DECL_THREAD_LOCAL_P (decl)
+		   ? TCTX_THREAD_STORAGE
+		   : TCTX_STATIC_STORAGE);
       verify_type_context (input_location, context, TREE_TYPE (decl));
     }
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index cb20329ceb5..61122e160a9 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -3608,6 +3608,10 @@ cp_build_indirect_ref_1 (location_t loc, tree ptr, ref_operator errorstring,
 	return TREE_OPERAND (pointer, 0);
       else
 	{
+	  if (!verify_type_context (loc, TCTX_AUTO_STORAGE, t,
+				    !(complain & tf_error)))
+	    return error_mark_node;
+
 	  tree ref = build1 (INDIRECT_REF, t, pointer);
 
 	  /* We *must* set TREE_READONLY when dereferencing a pointer to const,
diff --git a/gcc/target.h b/gcc/target.h
index d8f45fb99c3..2570b775c5e 100644
--- a/gcc/target.h
+++ b/gcc/target.h
@@ -217,6 +217,9 @@ const unsigned int VECT_COMPARE_COSTS = 1U << 0;
 /* The contexts in which the use of a type T can be checked by
    TARGET_VERIFY_TYPE_CONTEXT.  */
 enum type_context_kind {
+  /* Creeating objects of type T with auto storage duration. */
+  TCTX_AUTO_STORAGE,
+
   /* Directly measuring the size of T.  */
   TCTX_SIZEOF,
 
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
index 09dfacd222d..3ea786fe4b3 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
@@ -7,6 +7,7 @@ f (svbool_t *x, svint8_t *y)
 {
   *x = svptrue_b8 (); /* { dg-error {ACLE function '(svbool_t svptrue_b8\(\)|svptrue_b8)' requires ISA extension 'sve'} } */
   /* { dg-message {note: you can enable 'sve' using the command-line option '-march', or by using the 'target' attribute or pragma} "" { target *-*-* } .-1 } */
+  /* { dg-error "SVE type 'svbool_t' {aka '__SVBool_t'} used without SVE ISA enabled" "indirect" {target *-*-* } .-2 } */
   *x = svptrue_b8 ();
   *x = svptrue_b8 ();
   *x = svptrue_b8 ();
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
index 35ab07f1b49..6aef1a77f14 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
@@ -3,6 +3,6 @@
 void
 f (__SVBool_t *x, __SVBool_t *y)
 {
-  *x = *y; /* { dg-error {this operation requires the SVE ISA extension} } */
+  *x = *y; /* { dg-error {SVE type '__SVBool_t' used without SVE ISA enabled} } */
   *x = *y;
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
index 6e8d951b294..33e2069fcb0 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
@@ -3,6 +3,6 @@
 void
 f (__SVInt8_t *x, __SVInt8_t *y)
 {
-  *x = *y; /* { dg-error {this operation requires the SVE ISA extension} } */
+  *x = *y; /* { dg-error {SVE type '__SVInt8_t' used without SVE ISA enabled} } */
   *x = *y;
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
index d91ba40de14..7dc10528a17 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
@@ -8,5 +8,6 @@ void
 f (svbool_t *x, svint8_t *y)
 {
   *x = svptrue_b8 (); /* { dg-error {ACLE function '(svbool_t svptrue_b8\(\)|svptrue_b8)' is incompatible with the use of '-mgeneral-regs-only'} } */
+  /* { dg-error {SVE type 'svbool_t' {aka '__SVBool_t'} used without SVE ISA enabled} "" { target *-*-* } .-1 } */
   *y = svadd_m (*x, *y, 1);
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
index 663165f892d..0f1bad1734e 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
@@ -10,5 +10,5 @@ svbool_t return_bool ();
 void
 f (svbool_t *ptr)
 {
-  *ptr = return_bool (); /* { dg-error {'return_bool' requires the SVE ISA extension} } */
+  *ptr = return_bool (); /* { dg-error {SVE type 'svbool_t' used without SVE ISA enabled} } */
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
index 6d5823cfde1..2f9ebd827a7 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
@@ -10,5 +10,5 @@ svbool_t (*return_bool) ();
 void
 f (svbool_t *ptr)
 {
-  *ptr = return_bool (); /* { dg-error {calls to functions of type 'svbool_t\(\)' require the SVE ISA extension} } */
+  *ptr = return_bool (); /* { dg-error {SVE type 'svbool_t' used without SVE ISA enabled} } */
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
index a248bdbdbd9..979f98c11c8 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
@@ -10,6 +10,5 @@ void take_svuint8 (svuint8_t);
 void
 f (svuint8_t *ptr)
 {
-  take_svuint8 (*ptr); /* { dg-error {this operation requires the SVE ISA extension} } */
-  /* { dg-error {'take_svuint8' requires the SVE ISA extension} "" { target *-*-* } .-1 } */
+  take_svuint8 (*ptr); /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
index 6263b5acdec..990e28aae70 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
@@ -11,6 +11,5 @@ void take_svuint8_eventually (float, float, float, float,
 void
 f (svuint8_t *ptr)
 {
-  take_svuint8_eventually (0, 0, 0, 0, 0, 0, 0, 0, *ptr); /* { dg-error {this operation requires the SVE ISA extension} } */
-  /* { dg-error {arguments of type '(svuint8_t|__SVUint8_t)' require the SVE ISA extension} "" { target *-*-* } .-1 } */
+  take_svuint8_eventually (0, 0, 0, 0, 0, 0, 0, 0, *ptr); /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
index 85b68bb3881..f11de7017a2 100644
--- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
@@ -10,6 +10,5 @@ void unprototyped ();
 void
 f (svuint8_t *ptr)
 {
-  unprototyped (*ptr);  /* { dg-error {this operation requires the SVE ISA extension} } */
-  /* { dg-error {arguments of type '(svuint8_t|__SVUint8_t)' require the SVE ISA extension} "" { target *-*-* } .-1 } */
+  unprototyped (*ptr);  /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
new file mode 100644
index 00000000000..7b7f322fe8c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-prune-output "compilation terminated" } */
+
+#include <arm_sve.h>
+
+#pragma GCC target "+nosve"
+
+int
+f ()
+{
+  char a[12];
+  __SVInt8_t freq; /* { dg-error {SVE type '__SVInt8_t' used without SVE ISA enabled} } */
+  return __builtin_bcmp (&freq, &freq, 10); 
+}
+
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
  2021-11-09 10:09 [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error apinski
@ 2021-11-09 14:38 ` Richard Sandiford
  2022-12-07  2:21 ` Kewen.Lin
  1 sibling, 0 replies; 9+ messages in thread
From: Richard Sandiford @ 2021-11-09 14:38 UTC (permalink / raw)
  To: apinski--- via Gcc-patches; +Cc: apinski

apinski--- via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> From: Andrew Pinski <apinski@marvell.com>
>
> This fixes fully where SVE types were being used without sve being enabled.
> Instead of trying to fix it such that we error out during RTL time, it is
> better to error out in front-ends.  This expands verify_type_context to
> have a context of auto storage decl which is used for both auto storage
> decls and for indirection context.
>
> A few testcases needed to be updated for the new error message; they were
> already being rejected before hand.
>
> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>
> PR target/99657
> gcc/c/ChangeLog:
>
> 	* c-decl.c (finish_decl): Call verify_type_context
> 	for all decls and not just global_decls.
> 	* c-typeck.c (build_indirect_ref): Call verify_type_context
> 	to check to see if the type is ok to be used.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64-sve-builtins.cc (verify_type_context):
> 	Add TXTC_AUTO_STORAGE support
> 	* target.h (enum type_context_kind): Add TXTC_AUTO_STORAGE.
>
> gcc/cp/ChangeLog:
>
> 	* decl.c (cp_finish_decl): Call verify_type_context
> 	for all decls and not just global_decls.
> 	* typeck.c (cp_build_indirect_ref_1): Call verify_type_context
> 	to check to see if the type is ok to be used.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/sve/acle/general/nosve_1.c: Update test.
> 	* gcc.target/aarch64/sve/acle/general/nosve_4.c: Likewise.
> 	* gcc.target/aarch64/sve/acle/general/nosve_5.c: Likewise.
> 	* gcc.target/aarch64/sve/acle/general/nosve_6.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_2.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_3.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_4.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_5.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_6.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_9.c: New test.

The AArch64 bits look good to me.  I agree that this gives
better error messages than trying to track the error during
RTL time (although we still have to do that too in some cases).

Thanks,
Richard

> ---
>  gcc/c/c-decl.c                                    | 14 +++++++-------
>  gcc/c/c-typeck.c                                  |  2 ++
>  gcc/config/aarch64/aarch64-sve-builtins.cc        | 14 ++++++++++++++
>  gcc/cp/decl.c                                     | 10 ++++++----
>  gcc/cp/typeck.c                                   |  4 ++++
>  gcc/target.h                                      |  3 +++
>  .../gcc.target/aarch64/sve/acle/general/nosve_1.c |  1 +
>  .../gcc.target/aarch64/sve/acle/general/nosve_4.c |  2 +-
>  .../gcc.target/aarch64/sve/acle/general/nosve_5.c |  2 +-
>  .../gcc.target/aarch64/sve/acle/general/nosve_6.c |  1 +
>  .../gcc.target/aarch64/sve/pcs/nosve_2.c          |  2 +-
>  .../gcc.target/aarch64/sve/pcs/nosve_3.c          |  2 +-
>  .../gcc.target/aarch64/sve/pcs/nosve_4.c          |  3 +--
>  .../gcc.target/aarch64/sve/pcs/nosve_5.c          |  3 +--
>  .../gcc.target/aarch64/sve/pcs/nosve_6.c          |  3 +--
>  .../gcc.target/aarch64/sve/pcs/nosve_9.c          | 15 +++++++++++++++
>  16 files changed, 60 insertions(+), 21 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
>
> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> index 186fa1692c1..b3583622475 100644
> --- a/gcc/c/c-decl.c
> +++ b/gcc/c/c-decl.c
> @@ -5441,19 +5441,19 @@ finish_decl (tree decl, location_t init_loc, tree init,
>  
>    if (VAR_P (decl))
>      {
> +      type_context_kind context = TCTX_AUTO_STORAGE;
>        if (init && TREE_CODE (init) == CONSTRUCTOR)
>  	add_flexible_array_elts_to_size (decl, init);
>  
>        complete_flexible_array_elts (DECL_INITIAL (decl));
>  
>        if (is_global_var (decl))
> -	{
> -	  type_context_kind context = (DECL_THREAD_LOCAL_P (decl)
> -				       ? TCTX_THREAD_STORAGE
> -				       : TCTX_STATIC_STORAGE);
> -	  if (!verify_type_context (input_location, context, TREE_TYPE (decl)))
> -	    TREE_TYPE (decl) = error_mark_node;
> -	}
> +	context = (DECL_THREAD_LOCAL_P (decl)
> +		   ? TCTX_THREAD_STORAGE
> +		   : TCTX_STATIC_STORAGE);
> +
> +      if (!verify_type_context (input_location, context, TREE_TYPE (decl)))
> +	TREE_TYPE (decl) = error_mark_node;
>  
>        if (DECL_SIZE (decl) == NULL_TREE && TREE_TYPE (decl) != error_mark_node
>  	  && COMPLETE_TYPE_P (TREE_TYPE (decl)))
> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 782414f8c8c..e926b7c1964 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -2630,6 +2630,8 @@ build_indirect_ref (location_t loc, tree ptr, ref_operator errstring)
>        else
>  	{
>  	  tree t = TREE_TYPE (type);
> +	  if (!verify_type_context (loc, TCTX_AUTO_STORAGE, t))
> +	    return error_mark_node;
>  
>  	  ref = build1 (INDIRECT_REF, t, pointer);
>  
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc b/gcc/config/aarch64/aarch64-sve-builtins.cc
> index bc92213665c..1d5083bf9fa 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> @@ -3834,11 +3834,25 @@ bool
>  verify_type_context (location_t loc, type_context_kind context,
>  		     const_tree type, bool silent_p)
>  {
> +  static bool informed = false;
>    if (!sizeless_type_p (type))
>      return true;
>  
>    switch (context)
>      {
> +    case TCTX_AUTO_STORAGE:
> +      if (TARGET_SVE)
> +	return true;
> +      if (!silent_p && !informed)
> +	{
> +	  informed = true;
> +	  error_at (loc, "SVE type %qT used without SVE ISA enabled", type);
> +	  inform (loc, "you can enable SVE using the command-line"
> +		  " option %<-march%>, or by using the %<target%>"
> +		  " attribute or pragma");
> +	}
> +      return false;
> +
>      case TCTX_SIZEOF:
>      case TCTX_STATIC_STORAGE:
>        if (!silent_p)
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 947bbfc6637..1ffd11acae0 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -7975,11 +7975,13 @@ cp_finish_decl (tree decl, tree init, bool init_const_expr_p,
>        && DECL_INITIALIZED_IN_CLASS_P (decl))
>      check_static_variable_definition (decl, type);
>  
> -  if (!processing_template_decl && VAR_P (decl) && is_global_var (decl))
> +  if (!processing_template_decl && VAR_P (decl))
>      {
> -      type_context_kind context = (DECL_THREAD_LOCAL_P (decl)
> -				   ? TCTX_THREAD_STORAGE
> -				   : TCTX_STATIC_STORAGE);
> +      type_context_kind context = TCTX_AUTO_STORAGE;
> +      if (is_global_var (decl))
> +	context = (DECL_THREAD_LOCAL_P (decl)
> +		   ? TCTX_THREAD_STORAGE
> +		   : TCTX_STATIC_STORAGE);
>        verify_type_context (input_location, context, TREE_TYPE (decl));
>      }
>  
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index cb20329ceb5..61122e160a9 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -3608,6 +3608,10 @@ cp_build_indirect_ref_1 (location_t loc, tree ptr, ref_operator errorstring,
>  	return TREE_OPERAND (pointer, 0);
>        else
>  	{
> +	  if (!verify_type_context (loc, TCTX_AUTO_STORAGE, t,
> +				    !(complain & tf_error)))
> +	    return error_mark_node;
> +
>  	  tree ref = build1 (INDIRECT_REF, t, pointer);
>  
>  	  /* We *must* set TREE_READONLY when dereferencing a pointer to const,
> diff --git a/gcc/target.h b/gcc/target.h
> index d8f45fb99c3..2570b775c5e 100644
> --- a/gcc/target.h
> +++ b/gcc/target.h
> @@ -217,6 +217,9 @@ const unsigned int VECT_COMPARE_COSTS = 1U << 0;
>  /* The contexts in which the use of a type T can be checked by
>     TARGET_VERIFY_TYPE_CONTEXT.  */
>  enum type_context_kind {
> +  /* Creeating objects of type T with auto storage duration. */
> +  TCTX_AUTO_STORAGE,
> +
>    /* Directly measuring the size of T.  */
>    TCTX_SIZEOF,
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
> index 09dfacd222d..3ea786fe4b3 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
> @@ -7,6 +7,7 @@ f (svbool_t *x, svint8_t *y)
>  {
>    *x = svptrue_b8 (); /* { dg-error {ACLE function '(svbool_t svptrue_b8\(\)|svptrue_b8)' requires ISA extension 'sve'} } */
>    /* { dg-message {note: you can enable 'sve' using the command-line option '-march', or by using the 'target' attribute or pragma} "" { target *-*-* } .-1 } */
> +  /* { dg-error "SVE type 'svbool_t' {aka '__SVBool_t'} used without SVE ISA enabled" "indirect" {target *-*-* } .-2 } */
>    *x = svptrue_b8 ();
>    *x = svptrue_b8 ();
>    *x = svptrue_b8 ();
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
> index 35ab07f1b49..6aef1a77f14 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
> @@ -3,6 +3,6 @@
>  void
>  f (__SVBool_t *x, __SVBool_t *y)
>  {
> -  *x = *y; /* { dg-error {this operation requires the SVE ISA extension} } */
> +  *x = *y; /* { dg-error {SVE type '__SVBool_t' used without SVE ISA enabled} } */
>    *x = *y;
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
> index 6e8d951b294..33e2069fcb0 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
> @@ -3,6 +3,6 @@
>  void
>  f (__SVInt8_t *x, __SVInt8_t *y)
>  {
> -  *x = *y; /* { dg-error {this operation requires the SVE ISA extension} } */
> +  *x = *y; /* { dg-error {SVE type '__SVInt8_t' used without SVE ISA enabled} } */
>    *x = *y;
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
> index d91ba40de14..7dc10528a17 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
> @@ -8,5 +8,6 @@ void
>  f (svbool_t *x, svint8_t *y)
>  {
>    *x = svptrue_b8 (); /* { dg-error {ACLE function '(svbool_t svptrue_b8\(\)|svptrue_b8)' is incompatible with the use of '-mgeneral-regs-only'} } */
> +  /* { dg-error {SVE type 'svbool_t' {aka '__SVBool_t'} used without SVE ISA enabled} "" { target *-*-* } .-1 } */
>    *y = svadd_m (*x, *y, 1);
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
> index 663165f892d..0f1bad1734e 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
> @@ -10,5 +10,5 @@ svbool_t return_bool ();
>  void
>  f (svbool_t *ptr)
>  {
> -  *ptr = return_bool (); /* { dg-error {'return_bool' requires the SVE ISA extension} } */
> +  *ptr = return_bool (); /* { dg-error {SVE type 'svbool_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
> index 6d5823cfde1..2f9ebd827a7 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
> @@ -10,5 +10,5 @@ svbool_t (*return_bool) ();
>  void
>  f (svbool_t *ptr)
>  {
> -  *ptr = return_bool (); /* { dg-error {calls to functions of type 'svbool_t\(\)' require the SVE ISA extension} } */
> +  *ptr = return_bool (); /* { dg-error {SVE type 'svbool_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
> index a248bdbdbd9..979f98c11c8 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
> @@ -10,6 +10,5 @@ void take_svuint8 (svuint8_t);
>  void
>  f (svuint8_t *ptr)
>  {
> -  take_svuint8 (*ptr); /* { dg-error {this operation requires the SVE ISA extension} } */
> -  /* { dg-error {'take_svuint8' requires the SVE ISA extension} "" { target *-*-* } .-1 } */
> +  take_svuint8 (*ptr); /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
> index 6263b5acdec..990e28aae70 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
> @@ -11,6 +11,5 @@ void take_svuint8_eventually (float, float, float, float,
>  void
>  f (svuint8_t *ptr)
>  {
> -  take_svuint8_eventually (0, 0, 0, 0, 0, 0, 0, 0, *ptr); /* { dg-error {this operation requires the SVE ISA extension} } */
> -  /* { dg-error {arguments of type '(svuint8_t|__SVUint8_t)' require the SVE ISA extension} "" { target *-*-* } .-1 } */
> +  take_svuint8_eventually (0, 0, 0, 0, 0, 0, 0, 0, *ptr); /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
> index 85b68bb3881..f11de7017a2 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
> @@ -10,6 +10,5 @@ void unprototyped ();
>  void
>  f (svuint8_t *ptr)
>  {
> -  unprototyped (*ptr);  /* { dg-error {this operation requires the SVE ISA extension} } */
> -  /* { dg-error {arguments of type '(svuint8_t|__SVUint8_t)' require the SVE ISA extension} "" { target *-*-* } .-1 } */
> +  unprototyped (*ptr);  /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
> new file mode 100644
> index 00000000000..7b7f322fe8c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-prune-output "compilation terminated" } */
> +
> +#include <arm_sve.h>
> +
> +#pragma GCC target "+nosve"
> +
> +int
> +f ()
> +{
> +  char a[12];
> +  __SVInt8_t freq; /* { dg-error {SVE type '__SVInt8_t' used without SVE ISA enabled} } */
> +  return __builtin_bcmp (&freq, &freq, 10); 
> +}
> +

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
  2021-11-09 10:09 [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error apinski
  2021-11-09 14:38 ` Richard Sandiford
@ 2022-12-07  2:21 ` Kewen.Lin
  2022-12-07  9:16   ` Richard Sandiford
  1 sibling, 1 reply; 9+ messages in thread
From: Kewen.Lin @ 2022-12-07  2:21 UTC (permalink / raw)
  To: gcc-patches
  Cc: apinski, polacek, Joseph Myers, jason, nathan,
	Segher Boessenkool, Richard Sandiford

Hi,

In the recent discussion on how to make some built-in type only valid for
some target features efficiently[1], Andrew mentioned this patch which he
made previously (Thanks!).  I confirmed it can help rs6000 related issue,
and noticed PR99657 is still opened, so I think we still want this to
be reviewed.

Could some C/C++ FE experts help to review it?

Thanks in advance!

BR,
Kewen

[1] https://gcc.gnu.org/pipermail/gcc/2022-December/240220.html

on 2021/11/9 18:09, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
> 
> This fixes fully where SVE types were being used without sve being enabled.
> Instead of trying to fix it such that we error out during RTL time, it is
> better to error out in front-ends.  This expands verify_type_context to
> have a context of auto storage decl which is used for both auto storage
> decls and for indirection context.
> 
> A few testcases needed to be updated for the new error message; they were
> already being rejected before hand.
> 
> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.
> 
> PR target/99657
> gcc/c/ChangeLog:
> 
> 	* c-decl.c (finish_decl): Call verify_type_context
> 	for all decls and not just global_decls.
> 	* c-typeck.c (build_indirect_ref): Call verify_type_context
> 	to check to see if the type is ok to be used.
> 
> gcc/ChangeLog:
> 
> 	* config/aarch64/aarch64-sve-builtins.cc (verify_type_context):
> 	Add TXTC_AUTO_STORAGE support
> 	* target.h (enum type_context_kind): Add TXTC_AUTO_STORAGE.
> 
> gcc/cp/ChangeLog:
> 
> 	* decl.c (cp_finish_decl): Call verify_type_context
> 	for all decls and not just global_decls.
> 	* typeck.c (cp_build_indirect_ref_1): Call verify_type_context
> 	to check to see if the type is ok to be used.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/aarch64/sve/acle/general/nosve_1.c: Update test.
> 	* gcc.target/aarch64/sve/acle/general/nosve_4.c: Likewise.
> 	* gcc.target/aarch64/sve/acle/general/nosve_5.c: Likewise.
> 	* gcc.target/aarch64/sve/acle/general/nosve_6.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_2.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_3.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_4.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_5.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_6.c: Likewise.
> 	* gcc.target/aarch64/sve/pcs/nosve_9.c: New test.
> ---
>  gcc/c/c-decl.c                                    | 14 +++++++-------
>  gcc/c/c-typeck.c                                  |  2 ++
>  gcc/config/aarch64/aarch64-sve-builtins.cc        | 14 ++++++++++++++
>  gcc/cp/decl.c                                     | 10 ++++++----
>  gcc/cp/typeck.c                                   |  4 ++++
>  gcc/target.h                                      |  3 +++
>  .../gcc.target/aarch64/sve/acle/general/nosve_1.c |  1 +
>  .../gcc.target/aarch64/sve/acle/general/nosve_4.c |  2 +-
>  .../gcc.target/aarch64/sve/acle/general/nosve_5.c |  2 +-
>  .../gcc.target/aarch64/sve/acle/general/nosve_6.c |  1 +
>  .../gcc.target/aarch64/sve/pcs/nosve_2.c          |  2 +-
>  .../gcc.target/aarch64/sve/pcs/nosve_3.c          |  2 +-
>  .../gcc.target/aarch64/sve/pcs/nosve_4.c          |  3 +--
>  .../gcc.target/aarch64/sve/pcs/nosve_5.c          |  3 +--
>  .../gcc.target/aarch64/sve/pcs/nosve_6.c          |  3 +--
>  .../gcc.target/aarch64/sve/pcs/nosve_9.c          | 15 +++++++++++++++
>  16 files changed, 60 insertions(+), 21 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
> 
> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> index 186fa1692c1..b3583622475 100644
> --- a/gcc/c/c-decl.c
> +++ b/gcc/c/c-decl.c
> @@ -5441,19 +5441,19 @@ finish_decl (tree decl, location_t init_loc, tree init,
>  
>    if (VAR_P (decl))
>      {
> +      type_context_kind context = TCTX_AUTO_STORAGE;
>        if (init && TREE_CODE (init) == CONSTRUCTOR)
>  	add_flexible_array_elts_to_size (decl, init);
>  
>        complete_flexible_array_elts (DECL_INITIAL (decl));
>  
>        if (is_global_var (decl))
> -	{
> -	  type_context_kind context = (DECL_THREAD_LOCAL_P (decl)
> -				       ? TCTX_THREAD_STORAGE
> -				       : TCTX_STATIC_STORAGE);
> -	  if (!verify_type_context (input_location, context, TREE_TYPE (decl)))
> -	    TREE_TYPE (decl) = error_mark_node;
> -	}
> +	context = (DECL_THREAD_LOCAL_P (decl)
> +		   ? TCTX_THREAD_STORAGE
> +		   : TCTX_STATIC_STORAGE);
> +
> +      if (!verify_type_context (input_location, context, TREE_TYPE (decl)))
> +	TREE_TYPE (decl) = error_mark_node;
>  
>        if (DECL_SIZE (decl) == NULL_TREE && TREE_TYPE (decl) != error_mark_node
>  	  && COMPLETE_TYPE_P (TREE_TYPE (decl)))
> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 782414f8c8c..e926b7c1964 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -2630,6 +2630,8 @@ build_indirect_ref (location_t loc, tree ptr, ref_operator errstring)
>        else
>  	{
>  	  tree t = TREE_TYPE (type);
> +	  if (!verify_type_context (loc, TCTX_AUTO_STORAGE, t))
> +	    return error_mark_node;
>  
>  	  ref = build1 (INDIRECT_REF, t, pointer);
>  
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc b/gcc/config/aarch64/aarch64-sve-builtins.cc
> index bc92213665c..1d5083bf9fa 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> @@ -3834,11 +3834,25 @@ bool
>  verify_type_context (location_t loc, type_context_kind context,
>  		     const_tree type, bool silent_p)
>  {
> +  static bool informed = false;
>    if (!sizeless_type_p (type))
>      return true;
>  
>    switch (context)
>      {
> +    case TCTX_AUTO_STORAGE:
> +      if (TARGET_SVE)
> +	return true;
> +      if (!silent_p && !informed)
> +	{
> +	  informed = true;
> +	  error_at (loc, "SVE type %qT used without SVE ISA enabled", type);
> +	  inform (loc, "you can enable SVE using the command-line"
> +		  " option %<-march%>, or by using the %<target%>"
> +		  " attribute or pragma");
> +	}
> +      return false;
> +
>      case TCTX_SIZEOF:
>      case TCTX_STATIC_STORAGE:
>        if (!silent_p)
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 947bbfc6637..1ffd11acae0 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -7975,11 +7975,13 @@ cp_finish_decl (tree decl, tree init, bool init_const_expr_p,
>        && DECL_INITIALIZED_IN_CLASS_P (decl))
>      check_static_variable_definition (decl, type);
>  
> -  if (!processing_template_decl && VAR_P (decl) && is_global_var (decl))
> +  if (!processing_template_decl && VAR_P (decl))
>      {
> -      type_context_kind context = (DECL_THREAD_LOCAL_P (decl)
> -				   ? TCTX_THREAD_STORAGE
> -				   : TCTX_STATIC_STORAGE);
> +      type_context_kind context = TCTX_AUTO_STORAGE;
> +      if (is_global_var (decl))
> +	context = (DECL_THREAD_LOCAL_P (decl)
> +		   ? TCTX_THREAD_STORAGE
> +		   : TCTX_STATIC_STORAGE);
>        verify_type_context (input_location, context, TREE_TYPE (decl));
>      }
>  
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index cb20329ceb5..61122e160a9 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -3608,6 +3608,10 @@ cp_build_indirect_ref_1 (location_t loc, tree ptr, ref_operator errorstring,
>  	return TREE_OPERAND (pointer, 0);
>        else
>  	{
> +	  if (!verify_type_context (loc, TCTX_AUTO_STORAGE, t,
> +				    !(complain & tf_error)))
> +	    return error_mark_node;
> +
>  	  tree ref = build1 (INDIRECT_REF, t, pointer);
>  
>  	  /* We *must* set TREE_READONLY when dereferencing a pointer to const,
> diff --git a/gcc/target.h b/gcc/target.h
> index d8f45fb99c3..2570b775c5e 100644
> --- a/gcc/target.h
> +++ b/gcc/target.h
> @@ -217,6 +217,9 @@ const unsigned int VECT_COMPARE_COSTS = 1U << 0;
>  /* The contexts in which the use of a type T can be checked by
>     TARGET_VERIFY_TYPE_CONTEXT.  */
>  enum type_context_kind {
> +  /* Creeating objects of type T with auto storage duration. */
> +  TCTX_AUTO_STORAGE,
> +
>    /* Directly measuring the size of T.  */
>    TCTX_SIZEOF,
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
> index 09dfacd222d..3ea786fe4b3 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
> @@ -7,6 +7,7 @@ f (svbool_t *x, svint8_t *y)
>  {
>    *x = svptrue_b8 (); /* { dg-error {ACLE function '(svbool_t svptrue_b8\(\)|svptrue_b8)' requires ISA extension 'sve'} } */
>    /* { dg-message {note: you can enable 'sve' using the command-line option '-march', or by using the 'target' attribute or pragma} "" { target *-*-* } .-1 } */
> +  /* { dg-error "SVE type 'svbool_t' {aka '__SVBool_t'} used without SVE ISA enabled" "indirect" {target *-*-* } .-2 } */
>    *x = svptrue_b8 ();
>    *x = svptrue_b8 ();
>    *x = svptrue_b8 ();
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
> index 35ab07f1b49..6aef1a77f14 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
> @@ -3,6 +3,6 @@
>  void
>  f (__SVBool_t *x, __SVBool_t *y)
>  {
> -  *x = *y; /* { dg-error {this operation requires the SVE ISA extension} } */
> +  *x = *y; /* { dg-error {SVE type '__SVBool_t' used without SVE ISA enabled} } */
>    *x = *y;
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
> index 6e8d951b294..33e2069fcb0 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
> @@ -3,6 +3,6 @@
>  void
>  f (__SVInt8_t *x, __SVInt8_t *y)
>  {
> -  *x = *y; /* { dg-error {this operation requires the SVE ISA extension} } */
> +  *x = *y; /* { dg-error {SVE type '__SVInt8_t' used without SVE ISA enabled} } */
>    *x = *y;
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
> index d91ba40de14..7dc10528a17 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
> @@ -8,5 +8,6 @@ void
>  f (svbool_t *x, svint8_t *y)
>  {
>    *x = svptrue_b8 (); /* { dg-error {ACLE function '(svbool_t svptrue_b8\(\)|svptrue_b8)' is incompatible with the use of '-mgeneral-regs-only'} } */
> +  /* { dg-error {SVE type 'svbool_t' {aka '__SVBool_t'} used without SVE ISA enabled} "" { target *-*-* } .-1 } */
>    *y = svadd_m (*x, *y, 1);
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
> index 663165f892d..0f1bad1734e 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
> @@ -10,5 +10,5 @@ svbool_t return_bool ();
>  void
>  f (svbool_t *ptr)
>  {
> -  *ptr = return_bool (); /* { dg-error {'return_bool' requires the SVE ISA extension} } */
> +  *ptr = return_bool (); /* { dg-error {SVE type 'svbool_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
> index 6d5823cfde1..2f9ebd827a7 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
> @@ -10,5 +10,5 @@ svbool_t (*return_bool) ();
>  void
>  f (svbool_t *ptr)
>  {
> -  *ptr = return_bool (); /* { dg-error {calls to functions of type 'svbool_t\(\)' require the SVE ISA extension} } */
> +  *ptr = return_bool (); /* { dg-error {SVE type 'svbool_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
> index a248bdbdbd9..979f98c11c8 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
> @@ -10,6 +10,5 @@ void take_svuint8 (svuint8_t);
>  void
>  f (svuint8_t *ptr)
>  {
> -  take_svuint8 (*ptr); /* { dg-error {this operation requires the SVE ISA extension} } */
> -  /* { dg-error {'take_svuint8' requires the SVE ISA extension} "" { target *-*-* } .-1 } */
> +  take_svuint8 (*ptr); /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
> index 6263b5acdec..990e28aae70 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
> @@ -11,6 +11,5 @@ void take_svuint8_eventually (float, float, float, float,
>  void
>  f (svuint8_t *ptr)
>  {
> -  take_svuint8_eventually (0, 0, 0, 0, 0, 0, 0, 0, *ptr); /* { dg-error {this operation requires the SVE ISA extension} } */
> -  /* { dg-error {arguments of type '(svuint8_t|__SVUint8_t)' require the SVE ISA extension} "" { target *-*-* } .-1 } */
> +  take_svuint8_eventually (0, 0, 0, 0, 0, 0, 0, 0, *ptr); /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
> index 85b68bb3881..f11de7017a2 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
> @@ -10,6 +10,5 @@ void unprototyped ();
>  void
>  f (svuint8_t *ptr)
>  {
> -  unprototyped (*ptr);  /* { dg-error {this operation requires the SVE ISA extension} } */
> -  /* { dg-error {arguments of type '(svuint8_t|__SVUint8_t)' require the SVE ISA extension} "" { target *-*-* } .-1 } */
> +  unprototyped (*ptr);  /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
> new file mode 100644
> index 00000000000..7b7f322fe8c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-prune-output "compilation terminated" } */
> +
> +#include <arm_sve.h>
> +
> +#pragma GCC target "+nosve"
> +
> +int
> +f ()
> +{
> +  char a[12];
> +  __SVInt8_t freq; /* { dg-error {SVE type '__SVInt8_t' used without SVE ISA enabled} } */
> +  return __builtin_bcmp (&freq, &freq, 10); 
> +}
> +


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
  2022-12-07  2:21 ` Kewen.Lin
@ 2022-12-07  9:16   ` Richard Sandiford
  2022-12-07 11:29     ` Kewen.Lin
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Sandiford @ 2022-12-07  9:16 UTC (permalink / raw)
  To: Kewen.Lin
  Cc: gcc-patches, apinski, polacek, Joseph Myers, jason, nathan,
	Segher Boessenkool

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi,
>
> In the recent discussion on how to make some built-in type only valid for
> some target features efficiently[1], Andrew mentioned this patch which he
> made previously (Thanks!).  I confirmed it can help rs6000 related issue,
> and noticed PR99657 is still opened, so I think we still want this to
> be reviewed.

But does it work for things like:

    void f(foo_t *x, foo_t *y) { *x = *y; }

where no variables are being created with foo_t type?

That's not to say we shouldn't have the patch.  I'm just not sure
it can be the complete solution.

Thanks,
Richard

>
> Could some C/C++ FE experts help to review it?
>
> Thanks in advance!
>
> BR,
> Kewen
>
> [1] https://gcc.gnu.org/pipermail/gcc/2022-December/240220.html
>
> on 2021/11/9 18:09, apinski--- via Gcc-patches wrote:
>> From: Andrew Pinski <apinski@marvell.com>
>> 
>> This fixes fully where SVE types were being used without sve being enabled.
>> Instead of trying to fix it such that we error out during RTL time, it is
>> better to error out in front-ends.  This expands verify_type_context to
>> have a context of auto storage decl which is used for both auto storage
>> decls and for indirection context.
>> 
>> A few testcases needed to be updated for the new error message; they were
>> already being rejected before hand.
>> 
>> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>> 
>> PR target/99657
>> gcc/c/ChangeLog:
>> 
>> 	* c-decl.c (finish_decl): Call verify_type_context
>> 	for all decls and not just global_decls.
>> 	* c-typeck.c (build_indirect_ref): Call verify_type_context
>> 	to check to see if the type is ok to be used.
>> 
>> gcc/ChangeLog:
>> 
>> 	* config/aarch64/aarch64-sve-builtins.cc (verify_type_context):
>> 	Add TXTC_AUTO_STORAGE support
>> 	* target.h (enum type_context_kind): Add TXTC_AUTO_STORAGE.
>> 
>> gcc/cp/ChangeLog:
>> 
>> 	* decl.c (cp_finish_decl): Call verify_type_context
>> 	for all decls and not just global_decls.
>> 	* typeck.c (cp_build_indirect_ref_1): Call verify_type_context
>> 	to check to see if the type is ok to be used.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 	* gcc.target/aarch64/sve/acle/general/nosve_1.c: Update test.
>> 	* gcc.target/aarch64/sve/acle/general/nosve_4.c: Likewise.
>> 	* gcc.target/aarch64/sve/acle/general/nosve_5.c: Likewise.
>> 	* gcc.target/aarch64/sve/acle/general/nosve_6.c: Likewise.
>> 	* gcc.target/aarch64/sve/pcs/nosve_2.c: Likewise.
>> 	* gcc.target/aarch64/sve/pcs/nosve_3.c: Likewise.
>> 	* gcc.target/aarch64/sve/pcs/nosve_4.c: Likewise.
>> 	* gcc.target/aarch64/sve/pcs/nosve_5.c: Likewise.
>> 	* gcc.target/aarch64/sve/pcs/nosve_6.c: Likewise.
>> 	* gcc.target/aarch64/sve/pcs/nosve_9.c: New test.
>> ---
>>  gcc/c/c-decl.c                                    | 14 +++++++-------
>>  gcc/c/c-typeck.c                                  |  2 ++
>>  gcc/config/aarch64/aarch64-sve-builtins.cc        | 14 ++++++++++++++
>>  gcc/cp/decl.c                                     | 10 ++++++----
>>  gcc/cp/typeck.c                                   |  4 ++++
>>  gcc/target.h                                      |  3 +++
>>  .../gcc.target/aarch64/sve/acle/general/nosve_1.c |  1 +
>>  .../gcc.target/aarch64/sve/acle/general/nosve_4.c |  2 +-
>>  .../gcc.target/aarch64/sve/acle/general/nosve_5.c |  2 +-
>>  .../gcc.target/aarch64/sve/acle/general/nosve_6.c |  1 +
>>  .../gcc.target/aarch64/sve/pcs/nosve_2.c          |  2 +-
>>  .../gcc.target/aarch64/sve/pcs/nosve_3.c          |  2 +-
>>  .../gcc.target/aarch64/sve/pcs/nosve_4.c          |  3 +--
>>  .../gcc.target/aarch64/sve/pcs/nosve_5.c          |  3 +--
>>  .../gcc.target/aarch64/sve/pcs/nosve_6.c          |  3 +--
>>  .../gcc.target/aarch64/sve/pcs/nosve_9.c          | 15 +++++++++++++++
>>  16 files changed, 60 insertions(+), 21 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
>> 
>> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
>> index 186fa1692c1..b3583622475 100644
>> --- a/gcc/c/c-decl.c
>> +++ b/gcc/c/c-decl.c
>> @@ -5441,19 +5441,19 @@ finish_decl (tree decl, location_t init_loc, tree init,
>>  
>>    if (VAR_P (decl))
>>      {
>> +      type_context_kind context = TCTX_AUTO_STORAGE;
>>        if (init && TREE_CODE (init) == CONSTRUCTOR)
>>  	add_flexible_array_elts_to_size (decl, init);
>>  
>>        complete_flexible_array_elts (DECL_INITIAL (decl));
>>  
>>        if (is_global_var (decl))
>> -	{
>> -	  type_context_kind context = (DECL_THREAD_LOCAL_P (decl)
>> -				       ? TCTX_THREAD_STORAGE
>> -				       : TCTX_STATIC_STORAGE);
>> -	  if (!verify_type_context (input_location, context, TREE_TYPE (decl)))
>> -	    TREE_TYPE (decl) = error_mark_node;
>> -	}
>> +	context = (DECL_THREAD_LOCAL_P (decl)
>> +		   ? TCTX_THREAD_STORAGE
>> +		   : TCTX_STATIC_STORAGE);
>> +
>> +      if (!verify_type_context (input_location, context, TREE_TYPE (decl)))
>> +	TREE_TYPE (decl) = error_mark_node;
>>  
>>        if (DECL_SIZE (decl) == NULL_TREE && TREE_TYPE (decl) != error_mark_node
>>  	  && COMPLETE_TYPE_P (TREE_TYPE (decl)))
>> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
>> index 782414f8c8c..e926b7c1964 100644
>> --- a/gcc/c/c-typeck.c
>> +++ b/gcc/c/c-typeck.c
>> @@ -2630,6 +2630,8 @@ build_indirect_ref (location_t loc, tree ptr, ref_operator errstring)
>>        else
>>  	{
>>  	  tree t = TREE_TYPE (type);
>> +	  if (!verify_type_context (loc, TCTX_AUTO_STORAGE, t))
>> +	    return error_mark_node;
>>  
>>  	  ref = build1 (INDIRECT_REF, t, pointer);
>>  
>> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc b/gcc/config/aarch64/aarch64-sve-builtins.cc
>> index bc92213665c..1d5083bf9fa 100644
>> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
>> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
>> @@ -3834,11 +3834,25 @@ bool
>>  verify_type_context (location_t loc, type_context_kind context,
>>  		     const_tree type, bool silent_p)
>>  {
>> +  static bool informed = false;
>>    if (!sizeless_type_p (type))
>>      return true;
>>  
>>    switch (context)
>>      {
>> +    case TCTX_AUTO_STORAGE:
>> +      if (TARGET_SVE)
>> +	return true;
>> +      if (!silent_p && !informed)
>> +	{
>> +	  informed = true;
>> +	  error_at (loc, "SVE type %qT used without SVE ISA enabled", type);
>> +	  inform (loc, "you can enable SVE using the command-line"
>> +		  " option %<-march%>, or by using the %<target%>"
>> +		  " attribute or pragma");
>> +	}
>> +      return false;
>> +
>>      case TCTX_SIZEOF:
>>      case TCTX_STATIC_STORAGE:
>>        if (!silent_p)
>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
>> index 947bbfc6637..1ffd11acae0 100644
>> --- a/gcc/cp/decl.c
>> +++ b/gcc/cp/decl.c
>> @@ -7975,11 +7975,13 @@ cp_finish_decl (tree decl, tree init, bool init_const_expr_p,
>>        && DECL_INITIALIZED_IN_CLASS_P (decl))
>>      check_static_variable_definition (decl, type);
>>  
>> -  if (!processing_template_decl && VAR_P (decl) && is_global_var (decl))
>> +  if (!processing_template_decl && VAR_P (decl))
>>      {
>> -      type_context_kind context = (DECL_THREAD_LOCAL_P (decl)
>> -				   ? TCTX_THREAD_STORAGE
>> -				   : TCTX_STATIC_STORAGE);
>> +      type_context_kind context = TCTX_AUTO_STORAGE;
>> +      if (is_global_var (decl))
>> +	context = (DECL_THREAD_LOCAL_P (decl)
>> +		   ? TCTX_THREAD_STORAGE
>> +		   : TCTX_STATIC_STORAGE);
>>        verify_type_context (input_location, context, TREE_TYPE (decl));
>>      }
>>  
>> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
>> index cb20329ceb5..61122e160a9 100644
>> --- a/gcc/cp/typeck.c
>> +++ b/gcc/cp/typeck.c
>> @@ -3608,6 +3608,10 @@ cp_build_indirect_ref_1 (location_t loc, tree ptr, ref_operator errorstring,
>>  	return TREE_OPERAND (pointer, 0);
>>        else
>>  	{
>> +	  if (!verify_type_context (loc, TCTX_AUTO_STORAGE, t,
>> +				    !(complain & tf_error)))
>> +	    return error_mark_node;
>> +
>>  	  tree ref = build1 (INDIRECT_REF, t, pointer);
>>  
>>  	  /* We *must* set TREE_READONLY when dereferencing a pointer to const,
>> diff --git a/gcc/target.h b/gcc/target.h
>> index d8f45fb99c3..2570b775c5e 100644
>> --- a/gcc/target.h
>> +++ b/gcc/target.h
>> @@ -217,6 +217,9 @@ const unsigned int VECT_COMPARE_COSTS = 1U << 0;
>>  /* The contexts in which the use of a type T can be checked by
>>     TARGET_VERIFY_TYPE_CONTEXT.  */
>>  enum type_context_kind {
>> +  /* Creeating objects of type T with auto storage duration. */
>> +  TCTX_AUTO_STORAGE,
>> +
>>    /* Directly measuring the size of T.  */
>>    TCTX_SIZEOF,
>>  
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
>> index 09dfacd222d..3ea786fe4b3 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_1.c
>> @@ -7,6 +7,7 @@ f (svbool_t *x, svint8_t *y)
>>  {
>>    *x = svptrue_b8 (); /* { dg-error {ACLE function '(svbool_t svptrue_b8\(\)|svptrue_b8)' requires ISA extension 'sve'} } */
>>    /* { dg-message {note: you can enable 'sve' using the command-line option '-march', or by using the 'target' attribute or pragma} "" { target *-*-* } .-1 } */
>> +  /* { dg-error "SVE type 'svbool_t' {aka '__SVBool_t'} used without SVE ISA enabled" "indirect" {target *-*-* } .-2 } */
>>    *x = svptrue_b8 ();
>>    *x = svptrue_b8 ();
>>    *x = svptrue_b8 ();
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
>> index 35ab07f1b49..6aef1a77f14 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_4.c
>> @@ -3,6 +3,6 @@
>>  void
>>  f (__SVBool_t *x, __SVBool_t *y)
>>  {
>> -  *x = *y; /* { dg-error {this operation requires the SVE ISA extension} } */
>> +  *x = *y; /* { dg-error {SVE type '__SVBool_t' used without SVE ISA enabled} } */
>>    *x = *y;
>>  }
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
>> index 6e8d951b294..33e2069fcb0 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_5.c
>> @@ -3,6 +3,6 @@
>>  void
>>  f (__SVInt8_t *x, __SVInt8_t *y)
>>  {
>> -  *x = *y; /* { dg-error {this operation requires the SVE ISA extension} } */
>> +  *x = *y; /* { dg-error {SVE type '__SVInt8_t' used without SVE ISA enabled} } */
>>    *x = *y;
>>  }
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
>> index d91ba40de14..7dc10528a17 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/nosve_6.c
>> @@ -8,5 +8,6 @@ void
>>  f (svbool_t *x, svint8_t *y)
>>  {
>>    *x = svptrue_b8 (); /* { dg-error {ACLE function '(svbool_t svptrue_b8\(\)|svptrue_b8)' is incompatible with the use of '-mgeneral-regs-only'} } */
>> +  /* { dg-error {SVE type 'svbool_t' {aka '__SVBool_t'} used without SVE ISA enabled} "" { target *-*-* } .-1 } */
>>    *y = svadd_m (*x, *y, 1);
>>  }
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
>> index 663165f892d..0f1bad1734e 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_2.c
>> @@ -10,5 +10,5 @@ svbool_t return_bool ();
>>  void
>>  f (svbool_t *ptr)
>>  {
>> -  *ptr = return_bool (); /* { dg-error {'return_bool' requires the SVE ISA extension} } */
>> +  *ptr = return_bool (); /* { dg-error {SVE type 'svbool_t' used without SVE ISA enabled} } */
>>  }
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
>> index 6d5823cfde1..2f9ebd827a7 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_3.c
>> @@ -10,5 +10,5 @@ svbool_t (*return_bool) ();
>>  void
>>  f (svbool_t *ptr)
>>  {
>> -  *ptr = return_bool (); /* { dg-error {calls to functions of type 'svbool_t\(\)' require the SVE ISA extension} } */
>> +  *ptr = return_bool (); /* { dg-error {SVE type 'svbool_t' used without SVE ISA enabled} } */
>>  }
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
>> index a248bdbdbd9..979f98c11c8 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_4.c
>> @@ -10,6 +10,5 @@ void take_svuint8 (svuint8_t);
>>  void
>>  f (svuint8_t *ptr)
>>  {
>> -  take_svuint8 (*ptr); /* { dg-error {this operation requires the SVE ISA extension} } */
>> -  /* { dg-error {'take_svuint8' requires the SVE ISA extension} "" { target *-*-* } .-1 } */
>> +  take_svuint8 (*ptr); /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>>  }
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
>> index 6263b5acdec..990e28aae70 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_5.c
>> @@ -11,6 +11,5 @@ void take_svuint8_eventually (float, float, float, float,
>>  void
>>  f (svuint8_t *ptr)
>>  {
>> -  take_svuint8_eventually (0, 0, 0, 0, 0, 0, 0, 0, *ptr); /* { dg-error {this operation requires the SVE ISA extension} } */
>> -  /* { dg-error {arguments of type '(svuint8_t|__SVUint8_t)' require the SVE ISA extension} "" { target *-*-* } .-1 } */
>> +  take_svuint8_eventually (0, 0, 0, 0, 0, 0, 0, 0, *ptr); /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>>  }
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
>> index 85b68bb3881..f11de7017a2 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_6.c
>> @@ -10,6 +10,5 @@ void unprototyped ();
>>  void
>>  f (svuint8_t *ptr)
>>  {
>> -  unprototyped (*ptr);  /* { dg-error {this operation requires the SVE ISA extension} } */
>> -  /* { dg-error {arguments of type '(svuint8_t|__SVUint8_t)' require the SVE ISA extension} "" { target *-*-* } .-1 } */
>> +  unprototyped (*ptr);  /* { dg-error {SVE type 'svuint8_t' used without SVE ISA enabled} } */
>>  }
>> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
>> new file mode 100644
>> index 00000000000..7b7f322fe8c
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/sve/pcs/nosve_9.c
>> @@ -0,0 +1,15 @@
>> +/* { dg-do compile } */
>> +/* { dg-prune-output "compilation terminated" } */
>> +
>> +#include <arm_sve.h>
>> +
>> +#pragma GCC target "+nosve"
>> +
>> +int
>> +f ()
>> +{
>> +  char a[12];
>> +  __SVInt8_t freq; /* { dg-error {SVE type '__SVInt8_t' used without SVE ISA enabled} } */
>> +  return __builtin_bcmp (&freq, &freq, 10); 
>> +}
>> +

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
  2022-12-07  9:16   ` Richard Sandiford
@ 2022-12-07 11:29     ` Kewen.Lin
  2022-12-07 12:55       ` Richard Sandiford
  0 siblings, 1 reply; 9+ messages in thread
From: Kewen.Lin @ 2022-12-07 11:29 UTC (permalink / raw)
  To: richard.sandiford
  Cc: apinski, gcc-patches, polacek, Joseph Myers, jason, nathan,
	Segher Boessenkool

Hi Richard,

on 2022/12/7 17:16, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> Hi,
>>
>> In the recent discussion on how to make some built-in type only valid for
>> some target features efficiently[1], Andrew mentioned this patch which he
>> made previously (Thanks!).  I confirmed it can help rs6000 related issue,
>> and noticed PR99657 is still opened, so I think we still want this to
>> be reviewed.
> 
> But does it work for things like:
> 
>     void f(foo_t *x, foo_t *y) { *x = *y; }
> 
> where no variables are being created with foo_t type?
> 

I think it can work for this case as it touches build_indirect_ref.

> That's not to say we shouldn't have the patch.  I'm just not sure
> it can be the complete solution.

I'm not sure about that either, maybe Andrew have more insights.
But as you pointed out in [1], I doubted trying to find all invalid
uses of a built-in type is worthwhile, it seems catching those usual
cases is enough and practical.  So if this verify_type_context
framework can cover the most of uses, maybe it's a good direction
to go and extend.

[1] https://gcc.gnu.org/pipermail/gcc/2022-December/240218.html

BR,
Kewen

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
  2022-12-07 11:29     ` Kewen.Lin
@ 2022-12-07 12:55       ` Richard Sandiford
  2022-12-08  2:14         ` Kewen.Lin
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Sandiford @ 2022-12-07 12:55 UTC (permalink / raw)
  To: Kewen.Lin
  Cc: apinski, gcc-patches, polacek, Joseph Myers, jason, nathan,
	Segher Boessenkool

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi Richard,
>
> on 2022/12/7 17:16, Richard Sandiford wrote:
>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>> Hi,
>>>
>>> In the recent discussion on how to make some built-in type only valid for
>>> some target features efficiently[1], Andrew mentioned this patch which he
>>> made previously (Thanks!).  I confirmed it can help rs6000 related issue,
>>> and noticed PR99657 is still opened, so I think we still want this to
>>> be reviewed.
>> 
>> But does it work for things like:
>> 
>>     void f(foo_t *x, foo_t *y) { *x = *y; }
>> 
>> where no variables are being created with foo_t type?
>> 
>
> I think it can work for this case as it touches build_indirect_ref.

Ah, ok.  But indirecting through a pointer doesn't seem to match
TCTX_AUTO_STORAGE.

I guess another case is where there are global variables of the type
that you want to forbid, compiled while the target feature is enabled,
and then a function tries to access those variables with the target
feature locally disabled (through a pragma or attribute).  Does that
case work?

That's not an issue for SVE because global variables can't have
sizeless type.

>> That's not to say we shouldn't have the patch.  I'm just not sure
>> it can be the complete solution.
>
> I'm not sure about that either, maybe Andrew have more insights.
> But as you pointed out in [1], I doubted trying to find all invalid
> uses of a built-in type is worthwhile, it seems catching those usual
> cases is enough and practical.  So if this verify_type_context
> framework can cover the most of uses, maybe it's a good direction
> to go and extend.

IMO it depends on what we're trying to protect against.  If the
compiler can handle these types correctly even when the target feature
is disabled, and we're simply disallowing the types for policy rather
than correctness reasons, then maybe just handling the usual cases is
good enough.  But things are different if the compiler is going to ICE
or generate invalid code when something slips through.  In that case,
I think the niche cases matter too.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
  2022-12-07 12:55       ` Richard Sandiford
@ 2022-12-08  2:14         ` Kewen.Lin
  2022-12-08  7:43           ` Richard Sandiford
  0 siblings, 1 reply; 9+ messages in thread
From: Kewen.Lin @ 2022-12-08  2:14 UTC (permalink / raw)
  To: richard.sandiford
  Cc: apinski, gcc-patches, polacek, Joseph Myers, jason, nathan,
	Segher Boessenkool

on 2022/12/7 20:55, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> Hi Richard,
>>
>> on 2022/12/7 17:16, Richard Sandiford wrote:
>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>>> Hi,
>>>>
>>>> In the recent discussion on how to make some built-in type only valid for
>>>> some target features efficiently[1], Andrew mentioned this patch which he
>>>> made previously (Thanks!).  I confirmed it can help rs6000 related issue,
>>>> and noticed PR99657 is still opened, so I think we still want this to
>>>> be reviewed.
>>>
>>> But does it work for things like:
>>>
>>>     void f(foo_t *x, foo_t *y) { *x = *y; }
>>>
>>> where no variables are being created with foo_t type?
>>>
>>
>> I think it can work for this case as it touches build_indirect_ref.
> 
> Ah, ok.  But indirecting through a pointer doesn't seem to match
> TCTX_AUTO_STORAGE.
> 

Indeed. :)

> I guess another case is where there are global variables of the type
> that you want to forbid, compiled while the target feature is enabled,
> and then a function tries to access those variables with the target
> feature locally disabled (through a pragma or attribute).  Does that
> case work?
> 

Thanks for pointing out this, I tried with the below test case:

__vector_quad a1;
__vector_quad a2;

__attribute__((target("cpu=power8")))
void foo ()
{
  a2 = a3;
}

the verify_type_context doesn't catch it as you suspected, I think
it needs some enhancements somewhere.

> That's not an issue for SVE because global variables can't have
> sizeless type.
> 
>>> That's not to say we shouldn't have the patch.  I'm just not sure
>>> it can be the complete solution.
>>
>> I'm not sure about that either, maybe Andrew have more insights.
>> But as you pointed out in [1], I doubted trying to find all invalid
>> uses of a built-in type is worthwhile, it seems catching those usual
>> cases is enough and practical.  So if this verify_type_context
>> framework can cover the most of uses, maybe it's a good direction
>> to go and extend.
> 
> IMO it depends on what we're trying to protect against.  If the
> compiler can handle these types correctly even when the target feature
> is disabled, and we're simply disallowing the types for policy rather
> than correctness reasons, then maybe just handling the usual cases is
> good enough.  But things are different if the compiler is going to ICE
> or generate invalid code when something slips through.  In that case,
> I think the niche cases matter too.
> 

Thanks for the clarification, good point, I agree!  It means we still
need some handlings in movoo and movxo to avoid possible ICE, which can
still be caused by some cases like the above one or similar.  This
verify_type_context checking is only a nice add-on to improve the
diagnosis for invalid built-in type.  I'm going to fix the expanders,
it should be independent of this patch.

BR,
Kewen

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
  2022-12-08  2:14         ` Kewen.Lin
@ 2022-12-08  7:43           ` Richard Sandiford
  2022-12-08  9:15             ` Kewen.Lin
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Sandiford @ 2022-12-08  7:43 UTC (permalink / raw)
  To: Kewen.Lin
  Cc: apinski, gcc-patches, polacek, Joseph Myers, jason, nathan,
	Segher Boessenkool

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> on 2022/12/7 20:55, Richard Sandiford wrote:
>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>> Hi Richard,
>>>
>>> on 2022/12/7 17:16, Richard Sandiford wrote:
>>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>>>> Hi,
>>>>>
>>>>> In the recent discussion on how to make some built-in type only valid for
>>>>> some target features efficiently[1], Andrew mentioned this patch which he
>>>>> made previously (Thanks!).  I confirmed it can help rs6000 related issue,
>>>>> and noticed PR99657 is still opened, so I think we still want this to
>>>>> be reviewed.
>>>>
>>>> But does it work for things like:
>>>>
>>>>     void f(foo_t *x, foo_t *y) { *x = *y; }
>>>>
>>>> where no variables are being created with foo_t type?
>>>>
>>>
>>> I think it can work for this case as it touches build_indirect_ref.
>> 
>> Ah, ok.  But indirecting through a pointer doesn't seem to match
>> TCTX_AUTO_STORAGE.
>> 
>
> Indeed. :)
>
>> I guess another case is where there are global variables of the type
>> that you want to forbid, compiled while the target feature is enabled,
>> and then a function tries to access those variables with the target
>> feature locally disabled (through a pragma or attribute).  Does that
>> case work?
>> 
>
> Thanks for pointing out this, I tried with the below test case:
>
> __vector_quad a1;
> __vector_quad a2;
>
> __attribute__((target("cpu=power8")))
> void foo ()
> {
>   a2 = a3;
> }
>
> the verify_type_context doesn't catch it as you suspected, I think
> it needs some enhancements somewhere.

FWIW, another possible case is:

  foo_t f();
  void g(foo_t);
  void h() { g(f()); }

I'm not aware of any verify_type_context checks that would catch this
for SVE (since it's valid for SVE types).

Thanks,
Richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error
  2022-12-08  7:43           ` Richard Sandiford
@ 2022-12-08  9:15             ` Kewen.Lin
  0 siblings, 0 replies; 9+ messages in thread
From: Kewen.Lin @ 2022-12-08  9:15 UTC (permalink / raw)
  To: richard.sandiford
  Cc: apinski, gcc-patches, polacek, Joseph Myers, jason, nathan,
	Segher Boessenkool

on 2022/12/8 15:43, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> on 2022/12/7 20:55, Richard Sandiford wrote:
>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>>> Hi Richard,
>>>>
>>>> on 2022/12/7 17:16, Richard Sandiford wrote:
>>>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>>>>> Hi,
>>>>>>
>>>>>> In the recent discussion on how to make some built-in type only valid for
>>>>>> some target features efficiently[1], Andrew mentioned this patch which he
>>>>>> made previously (Thanks!).  I confirmed it can help rs6000 related issue,
>>>>>> and noticed PR99657 is still opened, so I think we still want this to
>>>>>> be reviewed.
>>>>>
>>>>> But does it work for things like:
>>>>>
>>>>>     void f(foo_t *x, foo_t *y) { *x = *y; }
>>>>>
>>>>> where no variables are being created with foo_t type?
>>>>>
>>>>
>>>> I think it can work for this case as it touches build_indirect_ref.
>>>
>>> Ah, ok.  But indirecting through a pointer doesn't seem to match
>>> TCTX_AUTO_STORAGE.
>>>
>>
>> Indeed. :)
>>
>>> I guess another case is where there are global variables of the type
>>> that you want to forbid, compiled while the target feature is enabled,
>>> and then a function tries to access those variables with the target
>>> feature locally disabled (through a pragma or attribute).  Does that
>>> case work?
>>>
>>
>> Thanks for pointing out this, I tried with the below test case:
>>
>> __vector_quad a1;
>> __vector_quad a2;
>>
>> __attribute__((target("cpu=power8")))
>> void foo ()
>> {
>>   a2 = a3;
>> }
>>
>> the verify_type_context doesn't catch it as you suspected, I think
>> it needs some enhancements somewhere.
> 
> FWIW, another possible case is:
> 
>   foo_t f();
>   void g(foo_t);
>   void h() { g(f()); }
> 
> I'm not aware of any verify_type_context checks that would catch this
> for SVE (since it's valid for SVE types).


OK, thanks for the information, MMA built-in types are not allowed to be
used in function arguments, by hacking with the restriction relaxing, I
confirm the verify_type_context check can't catch this case.

BR,
Kewen

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-12-08  9:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-09 10:09 [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error apinski
2021-11-09 14:38 ` Richard Sandiford
2022-12-07  2:21 ` Kewen.Lin
2022-12-07  9:16   ` Richard Sandiford
2022-12-07 11:29     ` Kewen.Lin
2022-12-07 12:55       ` Richard Sandiford
2022-12-08  2:14         ` Kewen.Lin
2022-12-08  7:43           ` Richard Sandiford
2022-12-08  9:15             ` Kewen.Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).