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?
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));
>>> +}
