[PATCH PR94678] aarch64: unexpected result with -mgeneral-regs-only and sve

[Richard Sandiford]

"Yangfei (Felix)" <felix.yang@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.

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  <felix.yang@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  <krebbel@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

Thanks,
Richard