Patchwork [2/3] canonicalize_loop_ivs should not generate unsigned types.

login
register
mail settings
Submitter Sebastian Pop
Date July 24, 2011, 7:48 p.m.
Message ID <CAFk3UF_+jn6bYJq+c6j88vU=K+g9w1nvtNzWcjxWF1qN9go1bg@mail.gmail.com>
Download mbox | patch
Permalink /patch/106569/
State New
Headers show

Comments

Sebastian Pop - July 24, 2011, 7:48 p.m.
On Sun, Jul 24, 2011 at 05:59, Richard Guenther
<richard.guenther@gmail.com> wrote:
> For two IVs with the same precision, one signed and one unsigned you choose
> signedness of the canonical IV based on the random order of PHIs - that doesn't
> look correct.
>
> I think what you should do here is use an unsigned type if any of the IVs with
> the current max precision is unsigned.

Attached is the updated patch.  Bootstrapped and tested on amd64-linux.
Ok for trunk?

Thanks,
Sebastian
Richard Guenther - July 25, 2011, 8:41 a.m.
On Sun, 24 Jul 2011, Sebastian Pop wrote:

> On Sun, Jul 24, 2011 at 05:59, Richard Guenther
> <richard.guenther@gmail.com> wrote:
> > For two IVs with the same precision, one signed and one unsigned you choose
> > signedness of the canonical IV based on the random order of PHIs - that doesn't
> > look correct.
> >
> > I think what you should do here is use an unsigned type if any of the IVs with
> > the current max precision is unsigned.
> 
> Attached is the updated patch.  Bootstrapped and tested on amd64-linux.
> Ok for trunk?

   for (psi = gsi_start_phis (loop->header);
        !gsi_end_p (psi); gsi_next (&psi))
@@ -1207,11 +1209,25 @@ canonicalize_loop_ivs (struct loop *loop, tree 
*nit,
bool bump_in_latch)
       gimple phi = gsi_stmt (psi);
       tree res = PHI_RESULT (phi);
 
-      if (is_gimple_reg (res) && TYPE_PRECISION (TREE_TYPE (res)) >
precision)
-       precision = TYPE_PRECISION (TREE_TYPE (res));
+      if (!is_gimple_reg (res))
+       continue;
+
+      type = TREE_TYPE (res);
+      if (TYPE_PRECISION (type) > precision)
+       {
+         precision = TYPE_PRECISION (type);
+         unsigned_p = TYPE_UNSIGNED (type);
+         continue;
+       }
+
+      if (TYPE_PRECISION (type) == precision
+         && TYPE_UNSIGNED (type))
+       unsigned_p = true;
     }

I think we also need to care for non-integral PHIs where TYPE_PRECISION
and TYPE_UNSIGNED are not applicable (seems the original code is also
buggy here?).  So, sth like

  type = TREE_TYPE (res);
  if (!is_gimple_reg (res)
      || !INTEGRAL_TYPE_P (type)
      || TYPE_PRECISION (type) < precision)
    continue;

  precision = TYPE_PRECISION (type);
  unsigned_p |= TYPE_UNSIGNED (type);
}

would be ok.

Ok with that change.

Thanks,
Richard.


> 
> Thanks,
> Sebastian
>
Sebastian Pop - July 25, 2011, 2:56 p.m.
On Mon, Jul 25, 2011 at 03:41, Richard Guenther <rguenther@suse.de> wrote:
> I think we also need to care for non-integral PHIs where TYPE_PRECISION
> and TYPE_UNSIGNED are not applicable (seems the original code is also
> buggy here?).  So, sth like
>
>  type = TREE_TYPE (res);
>  if (!is_gimple_reg (res)
>      || !INTEGRAL_TYPE_P (type)
>      || TYPE_PRECISION (type) < precision)
>    continue;
>
>  precision = TYPE_PRECISION (type);
>  unsigned_p |= TYPE_UNSIGNED (type);
> }
>

This would not work optimally on the following sequence:
unsigned char
signed short
as we would set the unsigned_p to true for the "unsigned char" and
then we would not reset the value of unsigned_p when the precision
increases.  So what about doing this instead:

  type = TREE_TYPE (res);
  if (!is_gimple_reg (res)
      || !INTEGRAL_TYPE_P (type)
      || TYPE_PRECISION (type) < precision)
    continue;

  if (TYPE_PRECISION (type) > precision)
    unsigned_p = TYPE_UNSIGNED (type);
  else
    unsigned_p |= TYPE_UNSIGNED (type);

  precision = TYPE_PRECISION (type);

Thanks,
Sebastian
Richard Guenther - July 25, 2011, 3:01 p.m.
On Mon, Jul 25, 2011 at 4:56 PM, Sebastian Pop <sebpop@gmail.com> wrote:
> On Mon, Jul 25, 2011 at 03:41, Richard Guenther <rguenther@suse.de> wrote:
>> I think we also need to care for non-integral PHIs where TYPE_PRECISION
>> and TYPE_UNSIGNED are not applicable (seems the original code is also
>> buggy here?).  So, sth like
>>
>>  type = TREE_TYPE (res);
>>  if (!is_gimple_reg (res)
>>      || !INTEGRAL_TYPE_P (type)
>>      || TYPE_PRECISION (type) < precision)
>>    continue;
>>
>>  precision = TYPE_PRECISION (type);
>>  unsigned_p |= TYPE_UNSIGNED (type);
>> }
>>
>
> This would not work optimally on the following sequence:
> unsigned char
> signed short
> as we would set the unsigned_p to true for the "unsigned char" and
> then we would not reset the value of unsigned_p when the precision
> increases.  So what about doing this instead:
>
>  type = TREE_TYPE (res);
>  if (!is_gimple_reg (res)
>      || !INTEGRAL_TYPE_P (type)
>      || TYPE_PRECISION (type) < precision)
>    continue;
>
>  if (TYPE_PRECISION (type) > precision)
>    unsigned_p = TYPE_UNSIGNED (type);
>  else
>    unsigned_p |= TYPE_UNSIGNED (type);
>
>  precision = TYPE_PRECISION (type);

Ah, indeed.  Yes, fine with me.

Thanks,
Richard.

> Thanks,
> Sebastian
>

Patch

From eab3e096781b0df1bda7129108eddbc73ebbfcd0 Mon Sep 17 00:00:00 2001
From: Sebastian Pop <sebpop@gmail.com>
Date: Sun, 24 Jul 2011 01:52:52 -0500
Subject: [PATCH 1/2] canonicalize_loop_ivs should not generate unsigned types.

2011-07-23  Sebastian Pop  <sebastian.pop@amd.com>

	* tree-ssa-loop-manip.c (canonicalize_loop_ivs): Build an unsigned
	iv only when the largest type is unsigned.  Do not call
	lang_hooks.types.type_for_size.

	* testsuite/libgomp.graphite/force-parallel-1.c: Un-xfail.
	* testsuite/libgomp.graphite/force-parallel-2.c: Adjust pattern.
---
 gcc/ChangeLog                                      |    6 +++++
 gcc/tree-ssa-loop-manip.c                          |   22 +++++++++++++++++--
 libgomp/ChangeLog                                  |    5 ++++
 .../testsuite/libgomp.graphite/force-parallel-1.c  |    2 +-
 .../testsuite/libgomp.graphite/force-parallel-2.c  |    2 +-
 5 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 31cd954..cd7c4b0 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,11 @@ 
 2011-07-23  Sebastian Pop  <sebastian.pop@amd.com>
 
+	* tree-ssa-loop-manip.c (canonicalize_loop_ivs): Build an unsigned
+	iv only when the largest type is unsigned.  Do not call
+	lang_hooks.types.type_for_size.
+
+2011-07-23  Sebastian Pop  <sebastian.pop@amd.com>
+
 	PR middle-end/47653
 	* graphite-scop-detection.c (graphite_can_represent_loop): Discard
 	loops using wrapping semantics.
diff --git a/gcc/tree-ssa-loop-manip.c b/gcc/tree-ssa-loop-manip.c
index 8176ed8..32082cf 100644
--- a/gcc/tree-ssa-loop-manip.c
+++ b/gcc/tree-ssa-loop-manip.c
@@ -1200,6 +1200,8 @@  canonicalize_loop_ivs (struct loop *loop, tree *nit, bool bump_in_latch)
   gimple stmt;
   edge exit = single_dom_exit (loop);
   gimple_seq stmts;
+  enum machine_mode mode;
+  bool unsigned_p = false;
 
   for (psi = gsi_start_phis (loop->header);
        !gsi_end_p (psi); gsi_next (&psi))
@@ -1207,11 +1209,25 @@  canonicalize_loop_ivs (struct loop *loop, tree *nit, bool bump_in_latch)
       gimple phi = gsi_stmt (psi);
       tree res = PHI_RESULT (phi);
 
-      if (is_gimple_reg (res) && TYPE_PRECISION (TREE_TYPE (res)) > precision)
-	precision = TYPE_PRECISION (TREE_TYPE (res));
+      if (!is_gimple_reg (res))
+	continue;
+
+      type = TREE_TYPE (res);
+      if (TYPE_PRECISION (type) > precision)
+	{
+	  precision = TYPE_PRECISION (type);
+	  unsigned_p = TYPE_UNSIGNED (type);
+	  continue;
+	}
+
+      if (TYPE_PRECISION (type) == precision
+	  && TYPE_UNSIGNED (type))
+	unsigned_p = true;
     }
 
-  type = lang_hooks.types.type_for_size (precision, 1);
+  mode = smallest_mode_for_size (precision, MODE_INT);
+  precision = GET_MODE_PRECISION (mode);
+  type = build_nonstandard_integer_type (precision, unsigned_p);
 
   if (original_precision != precision)
     {
diff --git a/libgomp/ChangeLog b/libgomp/ChangeLog
index 9225401..d5cd94d 100644
--- a/libgomp/ChangeLog
+++ b/libgomp/ChangeLog
@@ -1,3 +1,8 @@ 
+2011-07-23  Sebastian Pop  <sebastian.pop@amd.com>
+
+	* testsuite/libgomp.graphite/force-parallel-1.c: Un-xfail.
+	* testsuite/libgomp.graphite/force-parallel-2.c: Adjust pattern.
+
 2011-07-18  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	PR target/49541
diff --git a/libgomp/testsuite/libgomp.graphite/force-parallel-1.c b/libgomp/testsuite/libgomp.graphite/force-parallel-1.c
index 71ed332..7f043d8 100644
--- a/libgomp/testsuite/libgomp.graphite/force-parallel-1.c
+++ b/libgomp/testsuite/libgomp.graphite/force-parallel-1.c
@@ -23,7 +23,7 @@  int main(void)
 }
 
 /* Check that parallel code generation part make the right answer.  */
-/* { dg-final { scan-tree-dump-times "1 loops carried no dependency" 2 "graphite" { xfail *-*-* } } } */
+/* { dg-final { scan-tree-dump-times "1 loops carried no dependency" 2 "graphite" } } */
 /* { dg-final { cleanup-tree-dump "graphite" } } */
 /* { dg-final { scan-tree-dump-times "loopfn" 5 "optimized" } } */
 /* { dg-final { cleanup-tree-dump "parloops" } } */
diff --git a/libgomp/testsuite/libgomp.graphite/force-parallel-2.c b/libgomp/testsuite/libgomp.graphite/force-parallel-2.c
index 1ce0feb..03d8236 100644
--- a/libgomp/testsuite/libgomp.graphite/force-parallel-2.c
+++ b/libgomp/testsuite/libgomp.graphite/force-parallel-2.c
@@ -23,7 +23,7 @@  int main(void)
 }
 
 /* Check that parallel code generation part make the right answer.  */
-/* { dg-final { scan-tree-dump-times "2 loops carried no dependency" 1 "graphite" } } */
+/* { dg-final { scan-tree-dump-times "2 loops carried no dependency" 2 "graphite" } } */
 /* { dg-final { cleanup-tree-dump "graphite" } } */
 /* { dg-final { scan-tree-dump-times "loopfn" 5 "optimized" } } */
 /* { dg-final { cleanup-tree-dump "parloops" } } */
-- 
1.7.4.1