* [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).