diff mbox series

rs6000: Fix ICE in decompose_normal_address, at rtlanal.c:6403

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

Commit Message

Peter Bergner April 16, 2020, 1:21 p.m. UTC 
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.

Comments

will schmidt April 16, 2020, 3:11 p.m. UTC | #1 
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
Peter Bergner April 16, 2020, 4:55 p.m. UTC | #2 
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
Segher Boessenkool April 16, 2020, 10:21 p.m. UTC | #3 
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
Peter Bergner April 17, 2020, 4:32 a.m. UTC | #4 
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 mbox series

Patch

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