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

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

[Jiufu Guo]

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))
+	{
+	  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" } */
+
+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" } */
+
+void
+f (void)
+{
+  extern int x;
+  __asm__ volatile("#%a0" ::"X"(&x));
+}
-- 
2.25.1

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

[Kewen.Lin]

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?

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

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

BR,
Kewen

> +
> +void
> +f (void)
> +{
> +  extern int x;
> +  __asm__ volatile("#%a0" ::"X"(&x));
> +}

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

[Jiufu Guo]

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

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

[Kewen.Lin]

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?

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

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

[Jiufu Guo]

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

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

[Segher Boessenkool]

On Tue, May 14, 2024 at 11:00:38AM +0800, Jiufu Guo wrote:
> >> 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).

You almost never want to test for asm, and just give the same error you
would give in non-asm.  It is the same problem after all, and giving the
user the same error message is the most helpful thing to do!

It can be useful to not say "ICE", but it already is prevented from
doing that here.


Segher

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

[Jiufu Guo]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Tue, May 14, 2024 at 11:00:38AM +0800, Jiufu Guo wrote:
>> >> 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).
>
> You almost never want to test for asm, and just give the same error you
> would give in non-asm.  It is the same problem after all, and giving the
> user the same error message is the most helpful thing to do!
Yes, just as Kewen's comments. The testing on 'this_is_asm_operands' and
'address_operand' is not in good place.
The message emitting and it's checking chould be more straightforward,
something like:
/* emit error for user asm code, or fault in compiler. */
else if (TARGET_TOC)
  output_operand_lossage ("xxx");

I would update the patch for this.

BR,
Jeff(Jiufu) Guo

>
> It can be useful to not say "ICE", but it already is prevented from
> doing that here.
>
>
> Segher

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

[Segher Boessenkool]

Oh, btw:

On Tue, May 14, 2024 at 11:00:38AM +0800, Jiufu Guo wrote:
> >> --- 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))
> >> +	{
> >> +	  output_operand_lossage ("invalid expression as operand");
> >> +	  return;
> >> +	}

That error message is not so good.  Firstly, it typically *is* a valid
expression here, just not a correct expression to have for an address.
But, more generally and usefully, the error message should say *what* is
wrong about the expression (namely, it is not an address).

Most of the time you can use the same error message for asm and other
expressions, and you get a great message in all contexts.
operand_lossage already takes care of telling the user "you did
something foolish" for inline asm, or "ICE" if it is a compiler problem
instead.

In error messages you do not often know what caused the problem, so
just report on the facts you *do* know (and moreso with warnings, there
you typically only know something looks unusual).


Segher

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

[Jiufu Guo]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Oh, btw:
>
> On Tue, May 14, 2024 at 11:00:38AM +0800, Jiufu Guo wrote:
>> >> --- 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))
>> >> +	{
>> >> +	  output_operand_lossage ("invalid expression as operand");
>> >> +	  return;
>> >> +	}
>
> That error message is not so good.  Firstly, it typically *is* a valid
> expression here, just not a correct expression to have for an address.
> But, more generally and usefully, the error message should say *what* is
> wrong about the expression (namely, it is not an address).

Thanks so much for your great review!
Reference other messages, I'm wondering "invalid %%a value" may be
acceptable, or "invalid %%a address expression in TOC" maybe better.

>
> Most of the time you can use the same error message for asm and other
> expressions, and you get a great message in all contexts.
> operand_lossage already takes care of telling the user "you did
> something foolish" for inline asm, or "ICE" if it is a compiler problem
> instead.
>
> In error messages you do not often know what caused the problem, so
> just report on the facts you *do* know (and moreso with warnings, there
> you typically only know something looks unusual).
>
Thanks again for helpful comments!

BR,
Jeff(Jiufu) Guo.

>
> Segher

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

[Segher Boessenkool]

On Tue, May 14, 2024 at 05:53:56PM +0800, Jiufu Guo wrote:
> Thanks so much for your great review!
> Reference other messages, I'm wondering "invalid %%a value" may be
> acceptable, or "invalid %%a address expression in TOC" maybe better.

"%%a requires a memory operand"?  Maybe even print out the actual
operand given, too.


Segher

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

[Jiufu Guo]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Tue, May 14, 2024 at 05:53:56PM +0800, Jiufu Guo wrote:
>> Thanks so much for your great review!
>> Reference other messages, I'm wondering "invalid %%a value" may be
>> acceptable, or "invalid %%a address expression in TOC" maybe better.
>
> "%%a requires a memory operand"?  Maybe even print out the actual
> operand given, too.

Thanks! I updated the code using:
"%%a requires a memory reference operand", since the actual operand
is treated as the address.

BR,
Jeff(Jiufu) Guo

>
>
> Segher

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

[Jiufu Guo]

Hi,

Jiufu Guo <guojiufu@linux.ibm.com> writes:

> Hi,
>
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>
>> On Tue, May 14, 2024 at 05:53:56PM +0800, Jiufu Guo wrote:
>>> Thanks so much for your great review!
>>> Reference other messages, I'm wondering "invalid %%a value" may be
>>> acceptable, or "invalid %%a address expression in TOC" maybe better.
>>
>> "%%a requires a memory operand"?  Maybe even print out the actual
>> operand given, too.
>
> Thanks! I updated the code using:
> "%%a requires a memory reference operand", since the actual operand
> is treated as the address.

I suspect one thing here: if "%%a requires memory" is accurate vs.
"%%a requires a memory reference".

Reference the words from doc:
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Generic-Operand-Modifiers
a: Substitute a memory reference, with the actual operand treated as the
address.

And for below code:
'("#%a0" : :"m"(x))' is not accepted.

While '("#%a0" : :"r"(&x))' is ok.

So, it may be more accurate that: "%%a" as requirement of address of
memory.

Any comments?  Thanks!

BR,
Jeff(Jiufu) Guo

>
> BR,
> Jeff(Jiufu) Guo
>
>>
>>
>> Segher

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

[Segher Boessenkool]

Hi!

On Thu, May 16, 2024 at 02:56:49PM +0800, Jiufu Guo wrote:
> Jiufu Guo <guojiufu@linux.ibm.com> writes:
> > Segher Boessenkool <segher@kernel.crashing.org> writes:
> >> On Tue, May 14, 2024 at 05:53:56PM +0800, Jiufu Guo wrote:
> >>> Thanks so much for your great review!
> >>> Reference other messages, I'm wondering "invalid %%a value" may be
> >>> acceptable, or "invalid %%a address expression in TOC" maybe better.
> >>
> >> "%%a requires a memory operand"?  Maybe even print out the actual
> >> operand given, too.
> >
> > Thanks! I updated the code using:
> > "%%a requires a memory reference operand", since the actual operand
> > is treated as the address.
> 
> I suspect one thing here: if "%%a requires memory" is accurate vs.
> "%%a requires a memory reference".
> 
> Reference the words from doc:
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Generic-Operand-Modifiers
> a: Substitute a memory reference, with the actual operand treated as the
> address.
> 
> And for below code:
> '("#%a0" : :"m"(x))' is not accepted.

Yeah, it always confuses me.  Sorry.  The operand is the actual address.

> While '("#%a0" : :"r"(&x))' is ok.
> 
> So, it may be more accurate that: "%%a" as requirement of address of
> memory.

That sounds good yes.


Segher

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

[Segher Boessenkool]

Hi!

On Mon, May 13, 2024 at 10:57:12AM +0800, Jiufu Guo wrote:
> For PR96866, when gcc print asm code for modifier "%a" which requires
> an address operand,

It requires a *memory* operand, and it outputs its address.  This is a
generic modifier btw (not rs6000).

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

"non-address form"?  Every mem has an address.

But 'X' is not memory.  What is it at all?  Why do we use that when you
*have to* have mem here?

The code you add that tests for address_operand looks wrong.  I would
expect it to test the operand is memory, instead :-)


Segher

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

[Jiufu Guo]

Hi,

Thanks for your helpful comments!

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Mon, May 13, 2024 at 10:57:12AM +0800, Jiufu Guo wrote:
>> For PR96866, when gcc print asm code for modifier "%a" which requires
>> an address operand,
>
> It requires a *memory* operand, and it outputs its address.  This is a
> generic modifier btw (not rs6000).
Oh, yeap. it outputs the operands's address. I would update words like:
which requires an addressable 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.
>
> "non-address form"?  Every mem has an address.
>
> But 'X' is not memory.  What is it at all?  Why do we use that when you
> *have to* have mem here?
"X" allows any thing.  This is the reason why the code is *invalid*.
Other constraints("r/m") should be better than "X" for "%a".

>
> The code you add that tests for address_operand looks wrong.  I would
> expect it to test the operand is memory, instead :-)
I understand your concern. While there is a tricky work:
before invoking print_operand_address/output_address, the orignal
operand (which would be 'mem') is stripped to it's address.
So, 'address_operand' is tested for print_operand_address is targets.

While I also wonder if "address_operand" is really needed. Because
under the condition:
```
  else if (SYMBOL_REF_P (x) || GET_CODE (x) == CONST
	   || GET_CODE (x) == LABEL_REF)
    {
```
'x' is already known, it only could be: SYMBOL_REF/LABEL_REF or CONST.
I would update the patch for this.

Thanks for your comments.

BR,
Jeff(Jiufu) Guo

>
>
> Segher