Re: [PATCH] report message for operator %a on unaddressible exp

[Kewen.Lin]

Hi,

on 2024/5/13 10:57, Jiufu Guo wrote:
> Hi,
> 
> For PR96866, when gcc print asm code for modifier "%a" which requires
> an address operand, while the operand is with the constraint "X" which
> allow non-address form.  An error message would be reported to indicate
> the invalid asm operands.
> 
> Bootstrap&regtest pass on ppc64{,le}.
> Is this ok for trunk?
> 
> BR,
> Jeff(Jiufu Guo)
> 
>       PR target/96866
> 
> gcc/ChangeLog:
> 
>       * config/rs6000/rs6000.cc (print_operand_address):
> 
> gcc/testsuite/ChangeLog:
> 
>       * gcc.target/powerpc/pr96866-1.c: New test.
>       * gcc.target/powerpc/pr96866-2.c: New test.
> 
> ---
>  gcc/config/rs6000/rs6000.cc                  |  6 ++++++
>  gcc/testsuite/gcc.target/powerpc/pr96866-1.c | 15 +++++++++++++++
>  gcc/testsuite/gcc.target/powerpc/pr96866-2.c | 10 ++++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr96866-1.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr96866-2.c
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 117999613d8..50943d76f79 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -14659,6 +14659,12 @@ print_operand_address (FILE *file, rtx x)
>    else if (SYMBOL_REF_P (x) || GET_CODE (x) == CONST
>          || GET_CODE (x) == LABEL_REF)
>      {
> +      if (this_is_asm_operands && !address_operand (x, VOIDmode))

Do we really need this_is_asm_operands here?

> +     {
> +       output_operand_lossage ("invalid expression as operand");
> +       return;
> +     }
> +
>        output_addr_const (file, x);
>        if (small_data_operand (x, GET_MODE (x)))
>       fprintf (file, "@%s(%s)", SMALL_DATA_RELOC,
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr96866-1.c 
> b/gcc/testsuite/gcc.target/powerpc/pr96866-1.c
> new file mode 100644
> index 00000000000..6554a472a11
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr96866-1.c
> @@ -0,0 +1,15 @@
> +/* It's to verify no ICE here, ignore error messages about invalid 'asm'.  */
> +/* { dg-excess-errors "pr96866-2.c" } */
> +/* { dg-options "-fPIC -O2" } */

Nit: If these two options are required, it would be good to have a comment 
explaining it a bit
when it's not obvious.

> +
> +int x[2];
> +
> +int __attribute__ ((noipa))
> +f1 (void)
> +{
> +  int n;
> +  int *p = x;
> +  *p++;
> +  __asm__ volatile("ld %0, %a1" : "=r"(n) : "X"(p));
> +  return n;
> +}
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr96866-2.c 
> b/gcc/testsuite/gcc.target/powerpc/pr96866-2.c
> new file mode 100644
> index 00000000000..a5ec96f29dd
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr96866-2.c
> @@ -0,0 +1,10 @@
> +/* It's to verify no ICE here, ignore error messages about invalid 'asm'.  */
> +/* { dg-excess-errors "pr96866-2.c" } */
> +/* { dg-options "-fPIC -O2" } */

Ditto.

BR,
Kewen

> +
> +void
> +f (void)
> +{
> +  extern int x;
> +  __asm__ volatile("#%a0" ::"X"(&x));
> +}