Re: [PATCH V1] HIGH part of symbol ref is invalid for constant pool
Segher Boessenkool writes: Thanks a lot for your review! > Hi! > > On Tue, Jul 19, 2022 at 10:30:54PM +0800, Jiufu Guo wrote: >> In patch https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html, >> test case was not added. After more check, a testcase is added for it. >> >> The high part of the symbol address is invalid for the constant pool. > > Invalid, how so? Is there a PR related here? Thanks, I just opened PR106460 for this issue. > > But it is not particularly useful ever, either: we do not know two > different addresses will have the same HIGH unless we know the exact > address, and then we don't need HIGH anyway. > >> * config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): >> Return true for HIGH code rtx. > > * config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): Return true > for HIGH code rtx. > > Please don't wrap lines early: changelog lines are 80 positions long, > including the leading tab (which counts as eight positions). Thanks for your suggestion! > >> static bool >> rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x) >> { >> - if (GET_CODE (x) == HIGH >> - && GET_CODE (XEXP (x, 0)) == UNSPEC) >> + /* High part of a symbol ref/address can not be put into constant pool. >> e.g. >> + (high:DI (symbol_ref:DI ("var")..)) or >> + (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..) >> + (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12. >> */ >> + if (GET_CODE (x) == HIGH) >> return true; > > I'm not sure the new comment is helpful at all? Are these examples of > where the compiler (or assembler perhaps) will choke? I debugged this function with the source code from GCC bootstrap and regtest, and then figured out these examples. In the next version patch, I updated the comments a little, hope that is more meaningful. :-) > >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c >> @@ -0,0 +1,11 @@ >> +/* { dg-do compile { target powerpc*-*-* } } */ > > Everything in gcc.target/powerpc is target powerpc* always. Thanks! I would remove this line. > >> +/* { dg-options "-O1 -mdejagnu-cpu=power10" } */ >> +/* (high:DI (symbol_ref:DI ("var_48")..))) should not cause ICE. */ > > Ah, so there is an ICE, I see. Please open a PR, and mention that in > the testcase as well as in the commit message and changelog. Thanks! I should open PR ealry :) In the updated patch, a testcase is named as pr106460.c, and memtioned in commit message and changelog. > > I agree with what the patch does, it just needs a little more work :-) I submitted a new version patch: https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598980.html Thanks in advance for any comments! BR, Jeff(Jiufu) > > > Segher
Re: [PATCH V1] HIGH part of symbol ref is invalid for constant pool
Hi! On Tue, Jul 19, 2022 at 10:30:54PM +0800, Jiufu Guo wrote: > In patch https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html, > test case was not added. After more check, a testcase is added for it. > > The high part of the symbol address is invalid for the constant pool. Invalid, how so? Is there a PR related here? But it is not particularly useful ever, either: we do not know two different addresses will have the same HIGH unless we know the exact address, and then we don't need HIGH anyway. > * config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): > Return true for HIGH code rtx. * config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): Return true for HIGH code rtx. Please don't wrap lines early: changelog lines are 80 positions long, including the leading tab (which counts as eight positions). > static bool > rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x) > { > - if (GET_CODE (x) == HIGH > - && GET_CODE (XEXP (x, 0)) == UNSPEC) > + /* High part of a symbol ref/address can not be put into constant pool. > e.g. > + (high:DI (symbol_ref:DI ("var")..)) or > + (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..) > + (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12. > */ > + if (GET_CODE (x) == HIGH) > return true; I'm not sure the new comment is helpful at all? Are these examples of where the compiler (or assembler perhaps) will choke? > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c > @@ -0,0 +1,11 @@ > +/* { dg-do compile { target powerpc*-*-* } } */ Everything in gcc.target/powerpc is target powerpc* always. > +/* { dg-options "-O1 -mdejagnu-cpu=power10" } */ > +/* (high:DI (symbol_ref:DI ("var_48")..))) should not cause ICE. */ Ah, so there is an ICE, I see. Please open a PR, and mention that in the testcase as well as in the commit message and changelog. I agree with what the patch does, it just needs a little more work :-) Segher
Re: [PATCH V1] HIGH part of symbol ref is invalid for constant pool
"Kewen.Lin" writes: > Hi Jeff, > > on 2022/7/19 22:30, Jiufu Guo wrote: >> Hi, >> >> In patch https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html, >> test case was not added. After more check, a testcase is added for it. >> > > Good to see that you constructed one actual test case, nice! :) > >> The high part of the symbol address is invalid for the constant pool. >> In function rs6000_cannot_force_const_mem, we already return true for >> "HIGH with UNSPEC" rtx. Below are some examples also indicate the high >> part of a symbol_ref: >> (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc]) >> (high:DI (symbol_ref:DI ("var_1")..))) >> >> This patch updates rs6000_cannot_force_const_mem to return true for >> rtx with HIGH code. >> >> Bootstrapped and regtested on ppc64le and ppc64. >> Is it ok for trunk? > > I think this patch is OK with some nits below tweaked. Thanks so much for your time to review and helpful comments! I will update accordingly before commit. BR, Jeff(Jiufu) > >> >> BR, >> Jeff(Jiufu) >> >> >> gcc/ChangeLog: >> >> * config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): >> Return true for HIGH code rtx. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/powerpc/constpoolcheck.c: New test. >> >> --- >> gcc/config/rs6000/rs6000.cc | 7 +-- >> gcc/testsuite/gcc.target/powerpc/constpoolcheck.c | 11 +++ >> 2 files changed, 16 insertions(+), 2 deletions(-) >> create mode 100644 gcc/testsuite/gcc.target/powerpc/constpoolcheck.c >> >> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc >> index 0af2085adc0..d56832ebbfc 100644 >> --- a/gcc/config/rs6000/rs6000.cc >> +++ b/gcc/config/rs6000/rs6000.cc >> @@ -9704,8 +9704,11 @@ rs6000_init_stack_protect_guard (void) >> static bool >> rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x) >> { >> - if (GET_CODE (x) == HIGH >> - && GET_CODE (XEXP (x, 0)) == UNSPEC) >> + /* High part of a symbol ref/address can not be put into constant pool. >> e.g. > > Nit: two spaces after the period in "... pool.". Thanks! > >> + (high:DI (symbol_ref:DI ("var")..)) or > > Nit: You have one "or" at the end of the above line, I think it's better to > keep the below line consistent by either removing the above " or" or adding > one "or" at the end of the below line. Thanks! > >> + (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..) > > >> + (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12. >> */ >> + if (GET_CODE (x) == HIGH) >> return true; >> >>/* A TLS symbol in the TOC cannot contain a sum. */ >> diff --git a/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c >> b/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c >> new file mode 100644 >> index 000..ed7a994827b >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c > > Maybe it's good to name it to "const-pool-check.c" or "not-force-const-mem.c". Great sugguestion! Thanks. > >> @@ -0,0 +1,11 @@ >> +/* { dg-do compile { target powerpc*-*-* } } */ > > Nit: this "dg-do" line isn't needed since all here are default. Thanks for your comments! > > BR, > Kewen > >> +/* { dg-options "-O1 -mdejagnu-cpu=power10" } */ >> +/* (high:DI (symbol_ref:DI ("var_48")..))) should not cause ICE. */ >> +extern short var_48; >> +void >> +foo (double *r) >> +{ >> + if (var_48) >> +*r = 1234.5678; >> +} >> +
Re: [PATCH V1] HIGH part of symbol ref is invalid for constant pool
Hi Jeff, on 2022/7/19 22:30, Jiufu Guo wrote: > Hi, > > In patch https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html, > test case was not added. After more check, a testcase is added for it. > Good to see that you constructed one actual test case, nice! :) > The high part of the symbol address is invalid for the constant pool. > In function rs6000_cannot_force_const_mem, we already return true for > "HIGH with UNSPEC" rtx. Below are some examples also indicate the high > part of a symbol_ref: > (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc]) > (high:DI (symbol_ref:DI ("var_1")..))) > > This patch updates rs6000_cannot_force_const_mem to return true for > rtx with HIGH code. > > Bootstrapped and regtested on ppc64le and ppc64. > Is it ok for trunk? I think this patch is OK with some nits below tweaked. > > BR, > Jeff(Jiufu) > > > gcc/ChangeLog: > > * config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): > Return true for HIGH code rtx. > > gcc/testsuite/ChangeLog: > > * gcc.target/powerpc/constpoolcheck.c: New test. > > --- > gcc/config/rs6000/rs6000.cc | 7 +-- > gcc/testsuite/gcc.target/powerpc/constpoolcheck.c | 11 +++ > 2 files changed, 16 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/constpoolcheck.c > > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc > index 0af2085adc0..d56832ebbfc 100644 > --- a/gcc/config/rs6000/rs6000.cc > +++ b/gcc/config/rs6000/rs6000.cc > @@ -9704,8 +9704,11 @@ rs6000_init_stack_protect_guard (void) > static bool > rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x) > { > - if (GET_CODE (x) == HIGH > - && GET_CODE (XEXP (x, 0)) == UNSPEC) > + /* High part of a symbol ref/address can not be put into constant pool. > e.g. Nit: two spaces after the period in "... pool.". > + (high:DI (symbol_ref:DI ("var")..)) or Nit: You have one "or" at the end of the above line, I think it's better to keep the below line consistent by either removing the above " or" or adding one "or" at the end of the below line. > + (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..) > + (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12. > */ > + if (GET_CODE (x) == HIGH) > return true; > >/* A TLS symbol in the TOC cannot contain a sum. */ > diff --git a/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c > b/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c > new file mode 100644 > index 000..ed7a994827b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c Maybe it's good to name it to "const-pool-check.c" or "not-force-const-mem.c". > @@ -0,0 +1,11 @@ > +/* { dg-do compile { target powerpc*-*-* } } */ Nit: this "dg-do" line isn't needed since all here are default. BR, Kewen > +/* { dg-options "-O1 -mdejagnu-cpu=power10" } */ > +/* (high:DI (symbol_ref:DI ("var_48")..))) should not cause ICE. */ > +extern short var_48; > +void > +foo (double *r) > +{ > + if (var_48) > +*r = 1234.5678; > +} > +
[PATCH V1] HIGH part of symbol ref is invalid for constant pool
Hi, In patch https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html, test case was not added. After more check, a testcase is added for it. The high part of the symbol address is invalid for the constant pool. In function rs6000_cannot_force_const_mem, we already return true for "HIGH with UNSPEC" rtx. Below are some examples also indicate the high part of a symbol_ref: (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc]) (high:DI (symbol_ref:DI ("var_1")..))) This patch updates rs6000_cannot_force_const_mem to return true for rtx with HIGH code. Bootstrapped and regtested on ppc64le and ppc64. Is it ok for trunk? BR, Jeff(Jiufu) gcc/ChangeLog: * config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): Return true for HIGH code rtx. gcc/testsuite/ChangeLog: * gcc.target/powerpc/constpoolcheck.c: New test. --- gcc/config/rs6000/rs6000.cc | 7 +-- gcc/testsuite/gcc.target/powerpc/constpoolcheck.c | 11 +++ 2 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.target/powerpc/constpoolcheck.c diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc index 0af2085adc0..d56832ebbfc 100644 --- a/gcc/config/rs6000/rs6000.cc +++ b/gcc/config/rs6000/rs6000.cc @@ -9704,8 +9704,11 @@ rs6000_init_stack_protect_guard (void) static bool rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x) { - if (GET_CODE (x) == HIGH - && GET_CODE (XEXP (x, 0)) == UNSPEC) + /* High part of a symbol ref/address can not be put into constant pool. e.g. + (high:DI (symbol_ref:DI ("var")..)) or + (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..) + (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12. */ + if (GET_CODE (x) == HIGH) return true; /* A TLS symbol in the TOC cannot contain a sum. */ diff --git a/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c b/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c new file mode 100644 index 000..ed7a994827b --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/constpoolcheck.c @@ -0,0 +1,11 @@ +/* { dg-do compile { target powerpc*-*-* } } */ +/* { dg-options "-O1 -mdejagnu-cpu=power10" } */ +/* (high:DI (symbol_ref:DI ("var_48")..))) should not cause ICE. */ +extern short var_48; +void +foo (double *r) +{ + if (var_48) +*r = 1234.5678; +} + -- 2.17.1