Hi,
Thanks a lot for your helpful review!
"Kewen.Lin" <li...@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®test 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));
>> +}
