Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve
"Yangfei (Felix)" writes: > Hi, > >> -Original Message- >> From: Richard Sandiford [mailto:richard.sandif...@arm.com] >> Sent: Wednesday, April 22, 2020 6:03 PM >> To: Yangfei (Felix) >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral- >> regs-only and sve >> >> Mostly LGTM, just a couple of minor points: > > Thanks for the very careful code review. :-) > I think the revised patch fixed these points. > GCC builds OK and the newly added test still works. > Please help push if it's good to go. Thanks, pushed to master. Richard
RE: [PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve
Hi, > -Original Message- > From: Richard Sandiford [mailto:richard.sandif...@arm.com] > Sent: Wednesday, April 22, 2020 6:03 PM > To: Yangfei (Felix) > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral- > regs-only and sve > > Mostly LGTM, just a couple of minor points: Thanks for the very careful code review. :-) I think the revised patch fixed these points. GCC builds OK and the newly added test still works. Please help push if it's good to go. Felix 0001-aarch64-unexpected-result-with-mgeneral-regs-only-an.patch Description: 0001-aarch64-unexpected-result-with-mgeneral-regs-only-an.patch
Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve
Mostly LGTM, just a couple of minor points:
"Yangfei (Felix)" writes:
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 6e226cc..e956b69 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,23 @@
> +2020-04-22 Felix Yang
> +
> + PR target/94678
> + * config/aarch64/aarch64.h (TARGET_SVE, TARGET_SVE2):
> + Add && !TARGET_GENERAL_REGS_ONLY.
> + (TARGET_SVE2_AES, TARGET_SVE2_BITPERM, TARGET_SVE2_SHA3,
> + TARGET_SVE2_SM4): Add && TARGET_SVE2.
I agree using "TARGET_SVE2 &&" is just as good as adding
"!TARGET_GENERAL_REGS_ONLY &&", and arguably better. But I think
it would then be more consistent to add "TARGET_SVE &&" rather than
"!TARGET_GENERAL_REGS_ONLY &&" to TARGET_SVE2.
> @@ -558,6 +558,10 @@ static hash_table
> *function_table;
> when the required extension is disabled. */
> static bool reported_missing_extension_p;
>
> +/* True if we've already complained about attempts to use functions
> + which require registers that is missing. */
s/is/are/
> +static bool reported_missing_registers_p;
> +
> /* Record that TYPE is an ABI-defined SVE type that contains NUM_ZR SVE
> vectors
> and NUM_PR SVE predicates. MANGLED_NAME, if nonnull, is the ABI-defined
> mangling of the type. */
> @@ -657,6 +661,28 @@ report_missing_extension (location_t location, tree
> fndecl,
>reported_missing_extension_p = true;
> }
>
> +/* Check whether the registers required by SVE function fndecl are available.
> + SVE registers are not usable when -mgeneral-regs-only option is specified.
> + Report an error against LOCATION and return false if not. */
The "Report an error ..." sentence really forms a unit with the
"Check ..." sentence. I think it would probably be better to move
the intervening "SVE registers are not usable..." comment...
> +static bool
> +check_required_registers (location_t location, tree fndecl)
> +{
> + /* Avoid reporting a slew of messages for a single oversight. */
> + if (reported_missing_registers_p)
> +return false;
> +
> + if (TARGET_GENERAL_REGS_ONLY)
...here.
> +{
> + error_at (location,
> + "ACLE function %qD is incompatible with the use of %qs",
> + fndecl, "-mgeneral-regs-only");
> + reported_missing_registers_p = true;
> + return false;
> +}
> +
> + return true;
> +}
> +
> /* Check whether all the AARCH64_FL_* values in REQUIRED_EXTENSIONS are
> enabled, given that those extensions are required for function FNDECL.
> Report an error against LOCATION if not. */
> [...]
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.h
> b/gcc/config/aarch64/aarch64-sve-builtins.h
> index f7f06d2..5fdccb1 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.h
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.h
> @@ -658,6 +658,7 @@ public:
>
> private:
>unsigned long m_old_isa_flags;
> + bool old_general_regs_only;
The convention (not always followed!) is to use an "m_" prefix for
private member variables for classes.
[https://gcc.gnu.org/codingconventions.html#Cxx_Names]
So please add an "m_" prefix here for consistency with the other two
member variables.
Thanks,
Richard
>bool m_old_have_regs_of_mode[MAX_MACHINE_MODE];
> };
>
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index 8f08bad..3aa3e25 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -309,22 +309,22 @@ extern unsigned aarch64_architecture_version;
> #define TARGET_DOTPROD (TARGET_SIMD && AARCH64_ISA_DOTPROD)
>
> /* SVE instructions, enabled through +sve. */
> -#define TARGET_SVE (AARCH64_ISA_SVE)
> +#define TARGET_SVE (!TARGET_GENERAL_REGS_ONLY && AARCH64_ISA_SVE)
>
> /* SVE2 instructions, enabled through +sve2. */
> -#define TARGET_SVE2 (AARCH64_ISA_SVE2)
> +#define TARGET_SVE2 (!TARGET_GENERAL_REGS_ONLY && AARCH64_ISA_SVE2)
>
> /* SVE2 AES instructions, enabled through +sve2-aes. */
> -#define TARGET_SVE2_AES (AARCH64_ISA_SVE2_AES)
> +#define TARGET_SVE2_AES (TARGET_SVE2 && AARCH64_ISA_SVE2_AES)
>
> /* SVE2 BITPERM instructions, enabled through +sve2-bitperm. */
> -#define TARGET_SVE2_BITPERM (AARCH64_ISA_SVE2_BITPERM)
> +#define TARGET_SVE2_BITPERM (TARGET_SVE2 && AARCH64_ISA_SVE2_BITPERM)
>
> /* SVE2 SHA3 instructions, enabled through +sve2-sha3. */
> -#define TARGET_SVE2_SHA3 (AARCH64_ISA_SVE2_SHA3)
> +#define TARGET_SVE2_SHA3 (TARGET_SVE2 && AARCH64_ISA_SVE2_SHA3)
>
> /* SVE2 SM4 instructions, enabled through +sve2-sm4. */
> -#define TARGET_SVE2_SM4 (AARCH64_ISA_SVE2_SM4)
> +#define TARGET_SVE2_SM4 (TARGET_SVE2 && AARCH64_ISA_SVE2_SM4)
>
> /* ARMv8.3-A features. */
> #define TARGET_ARMV8_3 (AARCH64_ISA_V8_3)
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index be374fb..b8ed79c 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2020-04-22 Felix Yang
> +
> + PR target/94678
> + *
RE: [PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve
> -Original Message- > From: Richard Sandiford [mailto:richard.sandif...@arm.com] > Sent: Tuesday, April 21, 2020 6:11 PM > To: Yangfei (Felix) > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral- > regs-only and sve > > Should aarch64_sve::init_builtins ()/aarch64_sve::handle_arm_sve_h () be > guarded by TARGET_SVE? > > > > Could you please confirm that? > > Yeah, that's right. As Jakub says, the SVE stuff is (deliberately) registered > unconditionally because it's possible to switch SVE on and off later. Also, > protecting it with just TARGET_SVE would mean that we'd continue to > register SVE2 functions even if SVE2 isn't currently enabled. Well, I was thinking maybe we can call aarch64_sve::init_builtins () in aarch64_pragma_target_parse when SVE is switched on. But I think I will leave this alone. > So what matters is whether SVE is enabled at the point of use, not the point > of the #include. FWIW, arm_neon.h works the same way: the same > functions are defined regardless of what the current prevailing architecture > is, > and what matters is whether the necessary features are enabled when the > functions are called. (Inlining fails if they're not.) But because we're > implementing arm_sve.h directly in the compiler, we don't need the > overhead of a full target switch when defining the functions. > > And like you say, the second of the above tests makes sure that turning SVE > on later does indeed work, while the first makes sure that we get an > appropriate error if SVE is disabled at the point of use. Thanks for the details, it helps :-) I have modified accordingly and attached please find the adapted v2 patch. Bootstrap and tested on aarch64 Linux platform. Does it look better? Note that we need to disable TARGET_GENERAL_REGS_ONLY before register_builtin_types. Otherwise we got ICEs like: : internal compiler error: in register_builtin_types, at config/aarch64/aarch64-sve-builtins.cc:3336 0x185bcfb register_builtin_types ../../gcc-git/gcc/config/aarch64/aarch64-sve-builtins.cc:3332 0x185c467 aarch64_sve::init_builtins() ../../gcc-git/gcc/config/aarch64/aarch64-sve-builtins.cc:3375 0x17c075b aarch64_init_builtins ../../gcc-git/gcc/config/aarch64/aarch64.c:13086 , where TYPE_MODE (vectype) is BLKmode. The reason is targetm.vector_mode_supported_p (mode) and have_regs_of_mode[mode] are false when TARGET_GENERAL_REGS_ONLY is enabled. (gdb) bt #0 vector_type_mode (t=0xb7aa2f18) at ../../gcc-git/gcc/tree.c:13825 #1 0x01297ee0 in layout_type (type=0xb7aa2f18) at ../../gcc-git/gcc/stor-layout.c:2400 #2 0x016e19d8 in make_vector_type (innertype=0xb7aa2a80, nunits=..., mode=E_VNx16BImode) at ../../gcc-git/gcc/tree.c:9984 #3 0x016e79b4 in build_truth_vector_type_for_mode (nunits=..., mask_mode=E_VNx16BImode) at ../../gcc-git/gcc/tree.c:10929 #4 0x0185bb00 in aarch64_sve::register_builtin_types () at ../../gcc-git/gcc/config/aarch64/aarch64-sve-builtins.cc:3331 #5 0x0185c468 in aarch64_sve::init_builtins () at ../../gcc-git/gcc/config/aarch64/aarch64-sve-builtins.cc:3375 #6 0x017c075c in aarch64_init_builtins () at ../../gcc-git/gcc/config/aarch64/aarch64.c:13086 #7 0x00a69cb4 in c_define_builtins (va_list_ref_type_node=0xb79ea540, va_list_arg_type_node=0xb79e79d8) at ../../gcc-git/gcc/c-family/c-common.c:3973 Felix pr94678-v2.patch Description: pr94678-v2.patch
Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve
"Yangfei (Felix)" writes: >> -Original Message- >> From: Richard Sandiford [mailto:richard.sandif...@arm.com] >> Sent: Tuesday, April 21, 2020 4:01 PM >> To: Yangfei (Felix) >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral- >> regs-only and sve >> >> "Yangfei (Felix)" writes: >> > Hi, >> > >> > It looks like there are several issues out there for sve codegen with - >> mgeneral-regs-only. >> > I have created a bug for that: >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94678 >> > >> > We do ISA extension checks for SVE in >> check_required_extensions(aarch64-sve-builtins.cc). >> > I think we may also need to check -mgeneral-regs-only there and issue an >> error message when this option is specified. >> > This would be cheaper as compared with adding && >> TARGET_GENERAL_REGS_ONLY to TARGET_SVE and similar macros. >> >> We should probably do both. >> >> The middle end should never try to use vector patterns when the vector >> modes have been disabled by !have_regs_of_mode. But it's still wrong for >> the target to provide patterns that would inevitably lead to spill failure >> due to >> lack of registers. So I think we should check !TARGET_GENERAL_REGS_ONLY >> in TARGET_SVE. > > Yes, that's right. And I have a question here: > Should aarch64_sve::init_builtins ()/aarch64_sve::handle_arm_sve_h () be > guarded by TARGET_SVE? > > I mean in aarch64_init_builtins: > if (TARGET_SVE) > aarch64_sve::init_builtins (); > > and in aarch64_pragma_aarch64: > if (TARGET_SVE) > aarch64_sve::handle_arm_sve_h (); > > It looks to me that this is not wanted from the following two tests: > ./gcc.target/aarch64/sve/acle/general/nosve_1.c > ./gcc.target/aarch64/sve/acle/general/nosve_2.c > > Could you please confirm that? Yeah, that's right. As Jakub says, the SVE stuff is (deliberately) registered unconditionally because it's possible to switch SVE on and off later. Also, protecting it with just TARGET_SVE would mean that we'd continue to register SVE2 functions even if SVE2 isn't currently enabled. So what matters is whether SVE is enabled at the point of use, not the point of the #include. FWIW, arm_neon.h works the same way: the same functions are defined regardless of what the current prevailing architecture is, and what matters is whether the necessary features are enabled when the functions are called. (Inlining fails if they're not.) But because we're implementing arm_sve.h directly in the compiler, we don't need the overhead of a full target switch when defining the functions. And like you say, the second of the above tests makes sure that turning SVE on later does indeed work, while the first makes sure that we get an appropriate error if SVE is disabled at the point of use. Thanks, Richard
Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve
On Tue, Apr 21, 2020 at 08:37:09AM +, Yangfei (Felix) wrote:
> Yes, that's right. And I have a question here:
> Should aarch64_sve::init_builtins ()/aarch64_sve::handle_arm_sve_h () be
> guarded by TARGET_SVE?
>
> I mean in aarch64_init_builtins:
> if (TARGET_SVE)
> aarch64_sve::init_builtins ();
>
> and in aarch64_pragma_aarch64:
> if (TARGET_SVE)
> aarch64_sve::handle_arm_sve_h ();
If aarch64 supports target attribute/pragma, can't one function be target
("msve")
and another one not? Would something then register the builtins for the
functions that need it? And on the other side, if the builtins are already
registered because of some earlier "msve" function, something should ensure
there are no ICEs even in target ("mgeneral-regs-only") functions later.
Jakub
RE: [PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve
> -Original Message-
> From: Richard Sandiford [mailto:richard.sandif...@arm.com]
> Sent: Tuesday, April 21, 2020 4:01 PM
> To: Yangfei (Felix)
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral-
> regs-only and sve
>
> "Yangfei (Felix)" writes:
> > Hi,
> >
> > It looks like there are several issues out there for sve codegen with -
> mgeneral-regs-only.
> > I have created a bug for that:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94678
> >
> > We do ISA extension checks for SVE in
> check_required_extensions(aarch64-sve-builtins.cc).
> > I think we may also need to check -mgeneral-regs-only there and issue an
> error message when this option is specified.
> > This would be cheaper as compared with adding &&
> TARGET_GENERAL_REGS_ONLY to TARGET_SVE and similar macros.
>
> We should probably do both.
>
> The middle end should never try to use vector patterns when the vector
> modes have been disabled by !have_regs_of_mode. But it's still wrong for
> the target to provide patterns that would inevitably lead to spill failure
> due to
> lack of registers. So I think we should check !TARGET_GENERAL_REGS_ONLY
> in TARGET_SVE.
Yes, that's right. And I have a question here:
Should aarch64_sve::init_builtins ()/aarch64_sve::handle_arm_sve_h () be
guarded by TARGET_SVE?
I mean in aarch64_init_builtins:
if (TARGET_SVE)
aarch64_sve::init_builtins ();
and in aarch64_pragma_aarch64:
if (TARGET_SVE)
aarch64_sve::handle_arm_sve_h ();
It looks to me that this is not wanted from the following two tests:
./gcc.target/aarch64/sve/acle/general/nosve_1.c
./gcc.target/aarch64/sve/acle/general/nosve_2.c
Could you please confirm that?
> I guess the main danger is for instructions like ADDVL, ADDPL and CNT[BHWD]
> that do actually operate on general registers. Perhaps there'll be weird
> corner cases in which the compiler wants to know the VL even for -mgeneral-
> regs. I can't think of one off-hand though.
>
> If that becomes a problem, we can introduce a second macro to control the
> general register operations. But I think we can safely assume for now that
> one macro is enough.
OK, let's see.
> > Attached please find the proposed patch. Bootstrap and tested on
> aarch64 Linux platform.
> > Suggestions?
> >
> > Thanks,
> > Felix
> >
> > diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 721928d..c109d7b
> > 100644
> > --- a/gcc/ChangeLog
> > +++ b/gcc/ChangeLog
> > @@ -1,3 +1,11 @@
> > +2020-04-21 Felix Yang
> > +
> > + PR target/94678
> > + * config/aarch64/aarch64-sve-builtins.cc
> (check_required_extensions):
> > + Add check for TARGET_GENERAL_REGS_ONLY.
> > + (report_missing_extension): Print different error message under
> > + TARGET_GENERAL_REGS_ONLY.
> > +
> > 2020-04-20 Andreas Krebbel
> >
> > * config/s390/vector.md ("popcountv8hi2_vx", "popcountv4si2_vx")
> > diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc
> > b/gcc/config/aarch64/aarch64-sve-builtins.cc
> > index ca4a0eb..4a77db7 100644
> > --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> > +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> > @@ -649,11 +649,21 @@ report_missing_extension (location_t location,
> tree fndecl,
> >if (reported_missing_extension_p)
> > return;
> >
> > - error_at (location, "ACLE function %qD requires ISA extension %qs",
> > - fndecl, extension);
> > - inform (location, "you can enable %qs using the command-line"
> > - " option %<-march%>, or by using the %"
> > - " attribute or pragma", extension);
> > + if (TARGET_GENERAL_REGS_ONLY)
> > +{
> > + error_at (location, "ACLE function %qD requires ISA extension %qs"
> > + " which is incompatible with the use of %qs",
> > + fndecl, extension, "-mgeneral-regs-only");
> > +}
> > + else
> > +{
> > + error_at (location, "ACLE function %qD requires ISA extension %qs",
> > + fndecl, extension);
> > + inform (location, "you can enable %qs using the command-line"
> > + " option %<-march%>, or by using the %"
> > + " attribute or pragma", extension);
> > +}
> > +
> >reported_missing_extension_p = true; }
> >
> > @@ -666,7 +676,14 @@ check_required_extensions (location_t location,
> > tree fndecl, {
> >uint64_t m
Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve
"Yangfei (Felix)" writes:
> Hi,
>
> It looks like there are several issues out there for sve codegen with
> -mgeneral-regs-only.
> I have created a bug for that:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94678
>
> We do ISA extension checks for SVE in
> check_required_extensions(aarch64-sve-builtins.cc).
> I think we may also need to check -mgeneral-regs-only there and issue an
> error message when this option is specified.
> This would be cheaper as compared with adding && TARGET_GENERAL_REGS_ONLY
> to TARGET_SVE and similar macros.
We should probably do both.
The middle end should never try to use vector patterns when the vector
modes have been disabled by !have_regs_of_mode. But it's still wrong
for the target to provide patterns that would inevitably lead to spill
failure due to lack of registers. So I think we should check
!TARGET_GENERAL_REGS_ONLY in TARGET_SVE.
I guess the main danger is for instructions like ADDVL, ADDPL and
CNT[BHWD] that do actually operate on general registers. Perhaps
there'll be weird corner cases in which the compiler wants to know
the VL even for -mgeneral-regs. I can't think of one off-hand though.
If that becomes a problem, we can introduce a second macro to control
the general register operations. But I think we can safely assume for
now that one macro is enough.
> Attached please find the proposed patch. Bootstrap and tested on aarch64
> Linux platform.
> Suggestions?
>
> Thanks,
> Felix
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 721928d..c109d7b 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,11 @@
> +2020-04-21 Felix Yang
> +
> + PR target/94678
> + * config/aarch64/aarch64-sve-builtins.cc (check_required_extensions):
> + Add check for TARGET_GENERAL_REGS_ONLY.
> + (report_missing_extension): Print different error message under
> + TARGET_GENERAL_REGS_ONLY.
> +
> 2020-04-20 Andreas Krebbel
>
> * config/s390/vector.md ("popcountv8hi2_vx", "popcountv4si2_vx")
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc
> b/gcc/config/aarch64/aarch64-sve-builtins.cc
> index ca4a0eb..4a77db7 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> @@ -649,11 +649,21 @@ report_missing_extension (location_t location, tree
> fndecl,
>if (reported_missing_extension_p)
> return;
>
> - error_at (location, "ACLE function %qD requires ISA extension %qs",
> - fndecl, extension);
> - inform (location, "you can enable %qs using the command-line"
> - " option %<-march%>, or by using the %"
> - " attribute or pragma", extension);
> + if (TARGET_GENERAL_REGS_ONLY)
> +{
> + error_at (location, "ACLE function %qD requires ISA extension %qs"
> + " which is incompatible with the use of %qs",
> + fndecl, extension, "-mgeneral-regs-only");
> +}
> + else
> +{
> + error_at (location, "ACLE function %qD requires ISA extension %qs",
> + fndecl, extension);
> + inform (location, "you can enable %qs using the command-line"
> + " option %<-march%>, or by using the %"
> + " attribute or pragma", extension);
> +}
> +
>reported_missing_extension_p = true;
> }
>
> @@ -666,7 +676,14 @@ check_required_extensions (location_t location, tree
> fndecl,
> {
>uint64_t missing_extensions = required_extensions & ~aarch64_isa_flags;
>if (missing_extensions == 0)
> -return true;
> +{
> + if (TARGET_GENERAL_REGS_ONLY
> + && ((DECL_MD_FUNCTION_CODE (fndecl) & AARCH64_BUILTIN_CLASS)
> + == AARCH64_BUILTIN_SVE))
> + missing_extensions = required_extensions;
> + else
> + return true;
> +}
All functions processed here are SVE functions.
Rather than structure it as above, I think we should just have a
dedicated function for checking and reporting TARGET_GENERAL_REGS_ONLY,
with its own static variable to protect against streams of messages.
Maybe "check_required_registers"? We can then call it in the return
statement above, instead of always returning true.
That way, we'll prefer to give error messages about missing extensions,
which seems like the more fundamental requirement. We'll only mention
-mgeneral-regs-only if all extensions are present and the problem really
is that option.
I think we can then shorten the message to:
ACLE function %qs is incompatible with the use of %qs
Thanks,
Richard
