Patchwork Fix Ada testsuite failures introduced by my folding patch

login
register
mail settings
Submitter Jan Hubicka
Date Sept. 5, 2010, 2:19 p.m.
Message ID <20100905141956.GH31380@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/63830/
State New
Headers show

Comments

Jan Hubicka - Sept. 5, 2010, 2:19 p.m.
Hi,
my previous folding patch introduced several Ada testsuite failures.  In some cases
we replace a=b by a={1,2} that is no longer valid gimple.  Other case is:

.3194_2 = (void (*ada__tags__prim_ptr) (void)[1:1] * {ref-all}) &c390011_2__tableT + 32;

here I think the expression can be validized and is ommision in
maybe_fold_offset_to_address, but i did not put too much effort to figure out where it fails.

This patch adds test that result of fold_const_aggregate_ref is min_invariant
before using it.  The function can return constructors to allow self recursion
when handling multiple references.

In the first case disabling transform is the only correct thing to do. In the second case
this just turns ICE into suboptimal codegen.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* gimple-fold.c (maybe_fold_reference): Test that result of
	fold_const_aggregate_ref is gimple_min_invariant.
Gerald Pfeifer - Sept. 5, 2010, 2:53 p.m.
On Sun, 5 Sep 2010, Jan Hubicka wrote:
> my previous folding patch introduced several Ada testsuite failures.

Is there a non-Ada testcase that you could add, Honza?

Gerald
Jan Hubicka - Sept. 5, 2010, 3:16 p.m.
> On Sun, 5 Sep 2010, Jan Hubicka wrote:
> > my previous folding patch introduced several Ada testsuite failures.
> 
> Is there a non-Ada testcase that you could add, Honza?
no, I don't have any.  The testcases are array assignments, so having C equivalent
is tricky.  I tried version with struct and it does not trigger an ICE.

Honza
> 
> Gerald
Richard Guenther - Sept. 5, 2010, 5:22 p.m.
On Sun, 5 Sep 2010, Jan Hubicka wrote:

> Hi,
> my previous folding patch introduced several Ada testsuite failures.  In some cases
> we replace a=b by a={1,2} that is no longer valid gimple.  Other case is:
> 
> .3194_2 = (void (*ada__tags__prim_ptr) (void)[1:1] * {ref-all}) &c390011_2__tableT + 32;
> 
> here I think the expression can be validized and is ommision in
> maybe_fold_offset_to_address, but i did not put too much effort to figure out where it fails.
> 
> This patch adds test that result of fold_const_aggregate_ref is min_invariant
> before using it.  The function can return constructors to allow self recursion
> when handling multiple references.
> 
> In the first case disabling transform is the only correct thing to do. In the second case
> this just turns ICE into suboptimal codegen.
> 
> Bootstrapped/regtested x86_64-linux, OK?

Ok.

Thanks,
Richard.

> Honza
> 
> 	* gimple-fold.c (maybe_fold_reference): Test that result of
> 	fold_const_aggregate_ref is gimple_min_invariant.
> Index: gimple-fold.c
> ===================================================================
> --- gimple-fold.c	(revision 163862)
> +++ gimple-fold.c	(working copy)
> @@ -477,7 +477,8 @@ maybe_fold_reference (tree expr, bool is
>    tree result;
>  
>    if (!is_lhs
> -      && (result = fold_const_aggregate_ref (expr)))
> +      && (result = fold_const_aggregate_ref (expr))
> +      && is_gimple_min_invariant (result))
>      return result;
>  
>    /* ???  We might want to open-code the relevant remaining cases
> 
>
Eric Botcazou - Sept. 7, 2010, 5:25 p.m.
> 	* gimple-fold.c (maybe_fold_reference): Test that result of
> 	fold_const_aggregate_ref is gimple_min_invariant.

Has it been applied?  There are still several (6) ACATS failures on x86-64:
  http://gcc.gnu.org/ml/gcc-testresults/2010-09/msg00635.html
and even more on SPARC64 for example.
Jan Hubicka - Sept. 7, 2010, 9 p.m.
> > 	* gimple-fold.c (maybe_fold_reference): Test that result of
> > 	fold_const_aggregate_ref is gimple_min_invariant.
> 
> Has it been applied?  There are still several (6) ACATS failures on x86-64:
>   http://gcc.gnu.org/ml/gcc-testresults/2010-09/msg00635.html
> and even more on SPARC64 for example.

I applied it today, so it should be gone now.  I will double check.

Honza
> 
> -- 
> Eric Botcazou

Patch

Index: gimple-fold.c
===================================================================
--- gimple-fold.c	(revision 163862)
+++ gimple-fold.c	(working copy)
@@ -477,7 +477,8 @@  maybe_fold_reference (tree expr, bool is
   tree result;
 
   if (!is_lhs
-      && (result = fold_const_aggregate_ref (expr)))
+      && (result = fold_const_aggregate_ref (expr))
+      && is_gimple_min_invariant (result))
     return result;
 
   /* ???  We might want to open-code the relevant remaining cases