Message ID | 864fad37-13a3-9100-8cf7-899c01099d3e@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | rs6000: Fix ICE in decompose_normal_address, at rtlanal.c:6403 | expand |
On Thu, 2020-04-16 at 08:21 -0500, Peter Bergner via Gcc-patches wrote: > The ICE in PR93974 is caused by a bug in decompose address not being > able to > handle Altivec addresses the use AND: to strip off the bottom address > bits. > Rather than modify lra-constraints.c or rtlanal.c to solve this > generic > problem this late in the release cycle, I have decided to fix this in > target > code by defining the TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P target hook > to > reject mems with Altivec addresses from being used as equivalent > expressions. > I think this is fine, since Altivec addresses are legacy > addresses. I have > confirmed the following patch fixes the ICE and that we still get the > same > code generated for the test case below, that we got before my PR93658 > patch. > > This passed bootstrap and regression testing on powerpc64le-linux > with no > regressions. Ok for mainline? > > Peter > > > gcc/ > PR rtl-optimization/93974 > * config/rs6000/rs6000.c (TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P): Define. > (rs6000_cannot_substitute_mem_equiv_p): New function. > > gcc/testsuite/ > PR rtl-optimization/93974 > * g++.dg/pr93974.C: New test. > ok > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index 4defc1ab52b..a723503b4dc 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -1734,6 +1734,9 @@ static const struct attribute_spec rs6000_attribute_table[] = > > #undef TARGET_MANGLE_DECL_ASSEMBLER_NAME > #define TARGET_MANGLE_DECL_ASSEMBLER_NAME rs6000_mangle_decl_assembler_name > + > +#undef TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P > +#define TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P rs6000_cannot_substitute_mem_equiv_p > ok > /* Processor table. */ > @@ -26375,6 +26378,22 @@ rs6000_predict_doloop_p (struct loop *loop) > return true; > } > > +/* Implement TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P. */ > + > +static bool > +rs6000_cannot_substitute_mem_equiv_p (rtx mem) > +{ > + gcc_assert (MEM_P (mem)); > + > + /* curr_insn_transform()'s handling of subregs cannot handle altivec AND: > + type addresses, so don't allow MEMs with those address types to be > + substituted as an equivalent expression. See PR93974 for details. */ > + if (GET_CODE (XEXP (mem, 0)) == AND) > + return true; > + > + return false; > +} ok > + > struct gcc_target targetm = TARGET_INITIALIZER; > > #include "gt-rs6000.h" > diff --git a/gcc/testsuite/g++.dg/pr93974.C b/gcc/testsuite/g++.dg/pr93974.C > new file mode 100644 > index 00000000000..ea5f2f817c1 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr93974.C > @@ -0,0 +1,27 @@ > +/* { dg-do compile { target { powerpc*-*-linux* } } } */ > +/* { dg-require-effective-target powerpc_p8vector_ok } */ > +/* { dg-options "-mdejagnu-cpu=power8 -O3 -fstack-protector-strong" } */ > + > +class a { > + double b[2]; > +public: > + a(); > +}; > + > +class c { > +public: > + typedef a d; > + d m_fn1() { > + a e; > + return e; > + } > +}; > +template <typename f> void operator+(f, typename f::d); > +void g() { > + c connector; > + for (;;) { > + c cut; > + a h = cut.m_fn1(); > + connector + h; > + } > +} ok. lgtm. :-) thanks -Will
On 4/16/20 8:21 AM, Peter Bergner wrote: > This passed bootstrap and regression testing on powerpc64le-linux with no > regressions. Ok for mainline? This also just passed bootstrap and regtesting on (BE) powerpc64-linux running the testsuite in both 32-bit and 64-bit modes, with no regressions. Peter
Hi! On Thu, Apr 16, 2020 at 08:21:07AM -0500, Peter Bergner wrote: > The ICE in PR93974 is caused by a bug in decompose address not being able to > handle Altivec addresses the use AND: to strip off the bottom address bits. > Rather than modify lra-constraints.c or rtlanal.c to solve this generic > problem this late in the release cycle, I have decided to fix this in target > code by defining the TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P target hook to > reject mems with Altivec addresses from being used as equivalent expressions. > I think this is fine, since Altivec addresses are legacy addresses. I have > confirmed the following patch fixes the ICE and that we still get the same > code generated for the test case below, that we got before my PR93658 patch. Excellent :-) Just some very minor things: > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -1734,6 +1734,9 @@ static const struct attribute_spec rs6000_attribute_table[] = > > #undef TARGET_MANGLE_DECL_ASSEMBLER_NAME > #define TARGET_MANGLE_DECL_ASSEMBLER_NAME rs6000_mangle_decl_assembler_name > + > +#undef TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P > +#define TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P rs6000_cannot_substitute_mem_equiv_p This line gets too long, you could split it in two? (I did say "very minor", right?) > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr93974.C > @@ -0,0 +1,27 @@ > +/* { dg-do compile { target { powerpc*-*-linux* } } } */ > +/* { dg-require-effective-target powerpc_p8vector_ok } */ > +/* { dg-options "-mdejagnu-cpu=power8 -O3 -fstack-protector-strong" } */ Is there a reason to do this on Linux only? If not, you can just do { dg-do compile } ? Okay for trunk, however you choose to resolve those things. Thank you! Segher
On 4/16/20 5:21 PM, Segher Boessenkool wrote: >> +#undef TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P >> +#define TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P rs6000_cannot_substitute_mem_equiv_p > > This line gets too long, you could split it in two? Done. >> +/* { dg-do compile { target { powerpc*-*-linux* } } } */ >> +/* { dg-require-effective-target powerpc_p8vector_ok } */ >> +/* { dg-options "-mdejagnu-cpu=power8 -O3 -fstack-protector-strong" } */ > > Is there a reason to do this on Linux only? If not, you can just do > { dg-do compile } ? Ok, changed. I was trying to limit it to POWER and then thought that no other OS on POWER supports P8 vector, so I added that hunk, but I guess the dg-requires-effective-target is enough. > Okay for trunk, however you choose to resolve those things. Thank you! Thanks for the review. I just pushed the updated patch. Peter
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 4defc1ab52b..a723503b4dc 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -1734,6 +1734,9 @@ static const struct attribute_spec rs6000_attribute_table[] = #undef TARGET_MANGLE_DECL_ASSEMBLER_NAME #define TARGET_MANGLE_DECL_ASSEMBLER_NAME rs6000_mangle_decl_assembler_name + +#undef TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P +#define TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P rs6000_cannot_substitute_mem_equiv_p /* Processor table. */ @@ -26375,6 +26378,22 @@ rs6000_predict_doloop_p (struct loop *loop) return true; } +/* Implement TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P. */ + +static bool +rs6000_cannot_substitute_mem_equiv_p (rtx mem) +{ + gcc_assert (MEM_P (mem)); + + /* curr_insn_transform()'s handling of subregs cannot handle altivec AND: + type addresses, so don't allow MEMs with those address types to be + substituted as an equivalent expression. See PR93974 for details. */ + if (GET_CODE (XEXP (mem, 0)) == AND) + return true; + + return false; +} + struct gcc_target targetm = TARGET_INITIALIZER; #include "gt-rs6000.h" diff --git a/gcc/testsuite/g++.dg/pr93974.C b/gcc/testsuite/g++.dg/pr93974.C new file mode 100644 index 00000000000..ea5f2f817c1 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr93974.C @@ -0,0 +1,27 @@ +/* { dg-do compile { target { powerpc*-*-linux* } } } */ +/* { dg-require-effective-target powerpc_p8vector_ok } */ +/* { dg-options "-mdejagnu-cpu=power8 -O3 -fstack-protector-strong" } */ + +class a { + double b[2]; +public: + a(); +}; + +class c { +public: + typedef a d; + d m_fn1() { + a e; + return e; + } +}; +template <typename f> void operator+(f, typename f::d); +void g() { + c connector; + for (;;) { + c cut; + a h = cut.m_fn1(); + connector + h; + } +}