Fix loop interchange ICE on bitfields (PR tree-optimization/83337)

Message ID 20171209220818.GF2353@tucnak
State New
Headers show
Series
  • Fix loop interchange ICE on bitfields (PR tree-optimization/83337)
Related show

Commit Message

Jakub Jelinek Dec. 9, 2017, 10:08 p.m.
Hi!

compute_access_stride does:
  tree ref = DR_REF (dr);
  tree scev_base = build_fold_addr_expr (ref);
but that really isn't valid for bitfield drs, where we can't take addresses
of the bitfield.
As I understood this function only wants to compute strides, so constant bit
offsets shouldn't make a difference, therefore this patch attempts to deal
with those the cheapest way by just using the struct address it is component
of instead for the purposes of stride computation.
If the bitfield is in a VLA structure, we can use the
DECL_BIT_FIELD_REPRESENTATIVE, otherwise punt.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-12-09  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/83337
	* gimple-loop-interchange.cc (compute_access_stride): Handle bitfield DRs
	properly.

	* gcc.dg/tree-ssa/loop-interchange-14.c: New test.
	* gcc.dg/tree-ssa/loop-interchange-15.c: New test.


	Jakub

Comments

Richard Biener Dec. 10, 2017, 7:24 a.m. | #1
On December 9, 2017 11:08:18 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!

>

>compute_access_stride does:

>  tree ref = DR_REF (dr);

>  tree scev_base = build_fold_addr_expr (ref);

>but that really isn't valid for bitfield drs, where we can't take

>addresses

>of the bitfield.

>As I understood this function only wants to compute strides, so

>constant bit

>offsets shouldn't make a difference, therefore this patch attempts to

>deal

>with those the cheapest way by just using the struct address it is

>component

>of instead for the purposes of stride computation.

>If the bitfield is in a VLA structure, we can use the

>DECL_BIT_FIELD_REPRESENTATIVE, otherwise punt.

>

>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?


OK. 

Richard. 

>2017-12-09  Jakub Jelinek  <jakub@redhat.com>

>

>	PR tree-optimization/83337

>	* gimple-loop-interchange.cc (compute_access_stride): Handle bitfield

>DRs

>	properly.

>

>	* gcc.dg/tree-ssa/loop-interchange-14.c: New test.

>	* gcc.dg/tree-ssa/loop-interchange-15.c: New test.

>

>--- gcc/gimple-loop-interchange.cc.jj	2017-12-08 12:27:11.000000000

>+0100

>+++ gcc/gimple-loop-interchange.cc	2017-12-09 14:11:17.271026901 +0100

>@@ -1291,6 +1291,30 @@ compute_access_stride (struct loop *loop

>   gcc_assert (loop == bb->loop_father);

> 

>   tree ref = DR_REF (dr);

>+  if (TREE_CODE (ref) == COMPONENT_REF

>+      && DECL_BIT_FIELD (TREE_OPERAND (ref, 1)))

>+    {

>+      /* We can't take address of bitfields.  If the bitfield is at

>constant

>+	 offset from the start of the struct, just use address of the

>+	 struct, for analysis of the strides that shouldn't matter.  */

>+      if (!TREE_OPERAND (ref, 2)

>+	  || TREE_CODE (TREE_OPERAND (ref, 2)) == INTEGER_CST)

>+	ref = TREE_OPERAND (ref, 0);

>+      /* Otherwise, if we have a bit field representative, use that. 

>*/

>+      else if (DECL_BIT_FIELD_REPRESENTATIVE (TREE_OPERAND (ref, 1))

>+	       != NULL_TREE)

>+	{

>+	  tree repr = DECL_BIT_FIELD_REPRESENTATIVE (TREE_OPERAND (ref, 1));

>+	  ref = build3 (COMPONENT_REF, TREE_TYPE (repr), TREE_OPERAND (ref,

>0),

>+			repr, TREE_OPERAND (ref, 2));

>+	}

>+      /* Otherwise punt.  */

>+      else

>+	{

>+	  dr->aux = strides;

>+	  return;

>+	}

>+    }

>   tree scev_base = build_fold_addr_expr (ref);

>   tree scev = analyze_scalar_evolution (loop, scev_base);

> scev = instantiate_scev (loop_preheader_edge (loop_nest), loop, scev);

>--- gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-14.c.jj	2017-12-09

>14:03:38.567556128 +0100

>+++ gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-14.c	2017-12-09

>14:10:07.089889192 +0100

>@@ -0,0 +1,60 @@

>+/* PR tree-optimization/83337 */

>+/* { dg-do run { target int32plus } } */

>+/* { dg-options "-O2 -floop-interchange

>-fdump-tree-linterchange-details" } */

>+

>+/* Copied from graphite/interchange-5.c */

>+

>+#define DEBUG 0

>+#if DEBUG

>+#include <stdio.h>

>+#endif

>+

>+#define N 100

>+#define M 1111

>+struct S { int a : 3; int b : 17; int c : 12; };

>+struct S A[N][M];

>+

>+static int __attribute__((noinline))

>+foo (void)

>+{

>+  int i, j;

>+

>+  for( i = 0; i < M; i++)

>+    for( j = 0; j < N; j++)

>+      A[j][i].b = 5 * A[j][i].b;

>+

>+  return A[0][0].b + A[N-1][M-1].b;

>+}

>+

>+extern void abort ();

>+

>+static void __attribute__((noinline))

>+init (int i)

>+{

>+  int j;

>+

>+  for (j = 0; j < M; j++)

>+    A[i][j].b = 2;

>+}

>+

>+int

>+main (void)

>+{

>+  int i, j, res;

>+

>+  for (i = 0; i < N; i++)

>+    init (i);

>+

>+  res = foo ();

>+

>+#if DEBUG

>+  fprintf (stderr, "res = %d \n", res);

>+#endif

>+

>+  if (res != 20)

>+    abort ();

>+

>+  return 0;

>+}

>+

>+/* { dg-final { scan-tree-dump-times "Loop_pair<outer:., inner:.> is

>interchanged" 1 "linterchange"} } */

>--- gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-15.c.jj	2017-12-09

>16:43:44.469944977 +0100

>+++ gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-15.c	2017-12-09

>20:14:20.469419342 +0100

>@@ -0,0 +1,53 @@

>+/* PR tree-optimization/83337 */

>+/* { dg-do run { target int32plus } } */

>+/* { dg-options "-O2 -floop-interchange" } */

>+

>+/* Copied from graphite/interchange-5.c */

>+

>+#define DEBUG 0

>+#if DEBUG

>+#include <stdio.h>

>+#endif

>+

>+#define N 100

>+#define M 1111

>+

>+extern void abort ();

>+

>+static void __attribute__((noipa))

>+foo (int n)

>+{

>+  int i, j;

>+  struct S { char d[n]; int a : 3; int b : 17; int c : 12; };

>+  struct S A[N][M];

>+

>+  for (i = 0; i < N; i++)

>+    {

>+      asm volatile ("" : : "g" (&A[0][0]) : "memory");

>+      for (j = 0; j < M; j++)

>+	A[i][j].b = 2;

>+    }

>+  asm volatile ("" : : "g" (&A[0][0]) : "memory");

>+

>+  for (i = 0; i < M; i++)

>+    for (j = 0; j < N; j++)

>+      A[j][i].b = 5 * A[j][i].b;

>+

>+  asm volatile ("" : : "g" (&A[0][0]) : "memory");

>+  int res = A[0][0].b + A[N-1][M-1].b;

>+

>+#if DEBUG

>+  fprintf (stderr, "res = %d \n", res);

>+#endif

>+

>+  if (res != 20)

>+    abort ();

>+}

>+

>+int

>+main (void)

>+{

>+  foo (1);

>+  foo (8);

>+  return 0;

>+}

>

>	Jakub

Patch

--- gcc/gimple-loop-interchange.cc.jj	2017-12-08 12:27:11.000000000 +0100
+++ gcc/gimple-loop-interchange.cc	2017-12-09 14:11:17.271026901 +0100
@@ -1291,6 +1291,30 @@  compute_access_stride (struct loop *loop
   gcc_assert (loop == bb->loop_father);
 
   tree ref = DR_REF (dr);
+  if (TREE_CODE (ref) == COMPONENT_REF
+      && DECL_BIT_FIELD (TREE_OPERAND (ref, 1)))
+    {
+      /* We can't take address of bitfields.  If the bitfield is at constant
+	 offset from the start of the struct, just use address of the
+	 struct, for analysis of the strides that shouldn't matter.  */
+      if (!TREE_OPERAND (ref, 2)
+	  || TREE_CODE (TREE_OPERAND (ref, 2)) == INTEGER_CST)
+	ref = TREE_OPERAND (ref, 0);
+      /* Otherwise, if we have a bit field representative, use that.  */
+      else if (DECL_BIT_FIELD_REPRESENTATIVE (TREE_OPERAND (ref, 1))
+	       != NULL_TREE)
+	{
+	  tree repr = DECL_BIT_FIELD_REPRESENTATIVE (TREE_OPERAND (ref, 1));
+	  ref = build3 (COMPONENT_REF, TREE_TYPE (repr), TREE_OPERAND (ref, 0),
+			repr, TREE_OPERAND (ref, 2));
+	}
+      /* Otherwise punt.  */
+      else
+	{
+	  dr->aux = strides;
+	  return;
+	}
+    }
   tree scev_base = build_fold_addr_expr (ref);
   tree scev = analyze_scalar_evolution (loop, scev_base);
   scev = instantiate_scev (loop_preheader_edge (loop_nest), loop, scev);
--- gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-14.c.jj	2017-12-09 14:03:38.567556128 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-14.c	2017-12-09 14:10:07.089889192 +0100
@@ -0,0 +1,60 @@ 
+/* PR tree-optimization/83337 */
+/* { dg-do run { target int32plus } } */
+/* { dg-options "-O2 -floop-interchange -fdump-tree-linterchange-details" } */
+
+/* Copied from graphite/interchange-5.c */
+
+#define DEBUG 0
+#if DEBUG
+#include <stdio.h>
+#endif
+
+#define N 100
+#define M 1111
+struct S { int a : 3; int b : 17; int c : 12; };
+struct S A[N][M];
+
+static int __attribute__((noinline))
+foo (void)
+{
+  int i, j;
+
+  for( i = 0; i < M; i++)
+    for( j = 0; j < N; j++)
+      A[j][i].b = 5 * A[j][i].b;
+
+  return A[0][0].b + A[N-1][M-1].b;
+}
+
+extern void abort ();
+
+static void __attribute__((noinline))
+init (int i)
+{
+  int j;
+
+  for (j = 0; j < M; j++)
+    A[i][j].b = 2;
+}
+
+int
+main (void)
+{
+  int i, j, res;
+
+  for (i = 0; i < N; i++)
+    init (i);
+
+  res = foo ();
+
+#if DEBUG
+  fprintf (stderr, "res = %d \n", res);
+#endif
+
+  if (res != 20)
+    abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Loop_pair<outer:., inner:.> is interchanged" 1 "linterchange"} } */
--- gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-15.c.jj	2017-12-09 16:43:44.469944977 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-15.c	2017-12-09 20:14:20.469419342 +0100
@@ -0,0 +1,53 @@ 
+/* PR tree-optimization/83337 */
+/* { dg-do run { target int32plus } } */
+/* { dg-options "-O2 -floop-interchange" } */
+
+/* Copied from graphite/interchange-5.c */
+
+#define DEBUG 0
+#if DEBUG
+#include <stdio.h>
+#endif
+
+#define N 100
+#define M 1111
+
+extern void abort ();
+
+static void __attribute__((noipa))
+foo (int n)
+{
+  int i, j;
+  struct S { char d[n]; int a : 3; int b : 17; int c : 12; };
+  struct S A[N][M];
+
+  for (i = 0; i < N; i++)
+    {
+      asm volatile ("" : : "g" (&A[0][0]) : "memory");
+      for (j = 0; j < M; j++)
+	A[i][j].b = 2;
+    }
+  asm volatile ("" : : "g" (&A[0][0]) : "memory");
+
+  for (i = 0; i < M; i++)
+    for (j = 0; j < N; j++)
+      A[j][i].b = 5 * A[j][i].b;
+
+  asm volatile ("" : : "g" (&A[0][0]) : "memory");
+  int res = A[0][0].b + A[N-1][M-1].b;
+
+#if DEBUG
+  fprintf (stderr, "res = %d \n", res);
+#endif
+
+  if (res != 20)
+    abort ();
+}
+
+int
+main (void)
+{
+  foo (1);
+  foo (8);
+  return 0;
+}