Hi,
"Kewen.Lin" <li...@linux.ibm.com> writes:
> Hi,
>
> on 2024/5/14 11:00, Jiufu Guo wrote:
>> 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).
>
> 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));
>>>> +}
