Segher Boessenkool <seg...@kernel.crashing.org> 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
