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

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

Hi,

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

> Hi,
>
> on 2024/5/14 11:00, Jiufu Guo wrote:
>> 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).
>
> The called function "output_operand_lossage" already takes different
> actions for this_is_asm_operands and !this_is_asm_operands cases, so
> for this_is_asm_operands, it goes with error_for_asm and no ICE, no?
>
> And without this_is_asm_operands, if we adopt constraint X internally
> and hit this (it means it's already unexpected), isn't better to see
> the ICE instead of going further?
Yeap, exactly! "output_operand_lossage" could handle both user 'asm'
error and internal_error.  So it would be ok to call it directly just
for "gcc_assert(TARGET_TOC)" for this "if condition". Like:
```
      else if (TARGET_TOC)
	output_operand_lossage ("invalid expression as operand");
```
I would refine the patch.

Thanks again for your great comments.

BR,
Jeff(Jiufu) Guo

>
> BR,
> Kewen
>
>> 
>> 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));
>>>> +}