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

[Jiufu Guo <guojiufu@linux.ibm.com>]

Hi,

Thanks a lot for your helpful review!

"Kewen.Lin" <linkw@linux.ibm.com> writes:

> 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?
I understand your point: 
since in function 'print_operand_address' which supports not only user
asm code.  So, it maybe incorrect if 'x' is not an 'address_operand',
no matter this_is_asm_operands.

Here, 'this_is_asm_operands' is needed because it would be treated as an
user fault in asm-code (otherwise, internal_error in the compiler).

I notice one thing:
As what we need is emitting error for printing address if the address
can not be access directly.
So it would be better to emit message through 'output_operand_lossage'
just befor gcc_assert(TARGET_TOC).

Thanks a lot for your insight comment!

>
>> +	{
>> +	  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.

Good suggestion, thanks!
>
>> +
>> +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.
Thanks!

BR,
Jeff(Jiufu) Guo
>
> BR,
> Kewen
>
>> +
>> +void
>> +f (void)
>> +{
>> +  extern int x;
>> +  __asm__ volatile("#%a0" ::"X"(&x));
>> +}