> -----Original Message-----
> From: Richard Sandiford [mailto:richard.sandif...@arm.com]
> Sent: Tuesday, April 21, 2020 4:01 PM
> To: Yangfei (Felix) <felix.y...@huawei.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH PR94678] aarch64: unexpected result with -mgeneral-
> regs-only and sve
>
> "Yangfei (Felix)" <felix.y...@huawei.com> 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 <felix.y...@huawei.com>
> > +
> > + 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 <kreb...@linux.ibm.com>
> >
> > * 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 %<target%>"
> > - " 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 %<target%>"
> > + " 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
Good suggestion. Will do.
Thanks,
Felix
