| Message ID | 20240513025712.889169-1-guojiufu@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | report message for operator %a on unaddressible exp | expand |
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? > + { > + 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)); > +}
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
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
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®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)); >> +}
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®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)); >>> +}
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®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)); >>>> +}
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
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
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
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
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
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
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
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
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)); +}
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