[PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[Andrew_Pinski]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

Hi,
   The problem here is that the gimplifier can produce invalid gimple when 
the address is of an non addressable expression like a conditional 
expression.  When Alexandre fixed PR 20280, he introduced the issue of 
building addresses of the two operand of a conditional expression if we 
wantted either a lvalue or a rvalue, this introduced this latent bug of 
invalid gimple. 

Now my patch fixes PR 20280 in the front-end so we never get &(a?b:c), we 
always get a?&b:&c instead.  This patch also fixes the invalid gimple 
issue when needing a temp variable.  This patch also reverts part of the 
patch for PR 20280 so only when we want lvalue of a conditional, we take 
the address inside the conditional. 

The middle part of the patch is enough to fix the bug but we get worse 
code as we get * (a?&b:&c) which is not optimized currently.

OK? Bootstrapped and tested on i686-linux-gnu with no regressions.


Thanks,
Andrew Pinski

ChangeLog:

        * gimplifier.c (gimplify_cond_expr): Don't create *(a?&b:&c), if a 
rvalue is ok.
        (gimplify_addr_expr): Create one more extra variable if we have a 
temp decl.


cp/ChangeLog:

        * typeck.c (build_address): For COND_EXPR, create a?&b:&c instead 
of the plain
        &(a?b:c) so we don't get a temp variable in the gimplifier.

testsuite/ChangeLog:

        * gcc.c-torture/compile/complex-5.c: New test.


[-- Attachment #2: fixpr30132.diff.txt --]
[-- Type: text/plain, Size: 2224 bytes --]

Index: testsuite/gcc.c-torture/compile/complex-5.c
===================================================================
--- testsuite/gcc.c-torture/compile/complex-5.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/complex-5.c	(revision 0)
@@ -0,0 +1,13 @@
+/* PR 30132 */
+#define complex _Complex
+void testit(double complex* t, double* b)
+{
+  b[0] = t[0]==0.0?0.0:-t[0];
+}
+
+main(void)
+{
+  static double complex k = 5;
+  static double b;
+  testit(&k,&b);
+}
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 122896)
+++ cp/typeck.c	(working copy)
@@ -4060,6 +4060,14 @@ build_address (tree t)
   if (error_operand_p (t) || !cxx_mark_addressable (t))
     return error_mark_node;
 
+  if (TREE_CODE (t) == COND_EXPR)
+    {
+      tree ptrtype = build_pointer_type (TREE_TYPE (t));
+      return build3 (COND_EXPR, ptrtype, TREE_OPERAND (t, 0),
+		     build_address (TREE_OPERAND (t, 1)),
+		     build_address (TREE_OPERAND (t, 2)));
+    }
+
   addr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (t)), t);
 
   return addr;
Index: gimplify.c
===================================================================
--- gimplify.c	(revision 122896)
+++ gimplify.c	(working copy)
@@ -2436,7 +2436,7 @@ gimplify_cond_expr (tree *expr_p, tree *
     {
       tree result;
 
-      if ((fallback & fb_lvalue) == 0)
+      if (fallback != fb_lvalue)
 	{
 	  result = tmp2 = tmp = create_tmp_var (TREE_TYPE (expr), "iftmp");
 	  ret = GS_ALL_DONE;
@@ -3948,6 +3948,15 @@ gimplify_addr_expr (tree *expr_p, tree *
 	  if (TREE_CODE (op0) == INDIRECT_REF)
 	    goto do_indirect_ref;
 
+	  /* If we don't already have an addressable expression, create a new
+	     decl to hold it so we don't get non gimple as the decl which was
+	     holding this before was not marked as addressable already.  */
+	  if (TREE_CODE (op0) == VAR_DECL
+	      && DECL_GIMPLE_FORMAL_TEMP_P (op0)
+	      && !TREE_ADDRESSABLE (op0))
+	    TREE_OPERAND (expr, 0) = get_initialized_tmp_var (op0, pre_p,
+							      post_p);
+
 	  /* Make sure TREE_INVARIANT, TREE_CONSTANT, and TREE_SIDE_EFFECTS
 	     is set properly.  */
 	  recompute_tree_invariant_for_addr_expr (expr);

Re: [PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[Richard Guenther]

On 3/14/07, Andrew_Pinski@playstation.sony.com
<Andrew_Pinski@playstation.sony.com> wrote:
> Hi,
>    The problem here is that the gimplifier can produce invalid gimple when
> the address is of an non addressable expression like a conditional
> expression.  When Alexandre fixed PR 20280, he introduced the issue of
> building addresses of the two operand of a conditional expression if we
> wantted either a lvalue or a rvalue, this introduced this latent bug of
> invalid gimple.
>
> Now my patch fixes PR 20280 in the front-end so we never get &(a?b:c), we
> always get a?&b:&c instead.  This patch also fixes the invalid gimple
> issue when needing a temp variable.  This patch also reverts part of the
> patch for PR 20280 so only when we want lvalue of a conditional, we take
> the address inside the conditional.
>
> The middle part of the patch is enough to fix the bug but we get worse
> code as we get * (a?&b:&c) which is not optimized currently.

There's a patch pending for this:

http://gcc.gnu.org/ml/gcc-patches/2007-02/msg01331.html

Richard.

Re: [PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[Richard Henderson]

On Wed, Mar 14, 2007 at 02:58:28PM -0800, Andrew_Pinski@PlayStation.Sony.Com wrote:
> +	  /* If we don't already have an addressable expression, create a new
> +	     decl to hold it so we don't get non gimple as the decl which was
> +	     holding this before was not marked as addressable already.  */
> +	  if (TREE_CODE (op0) == VAR_DECL
> +	      && DECL_GIMPLE_FORMAL_TEMP_P (op0)
> +	      && !TREE_ADDRESSABLE (op0))
> +	    TREE_OPERAND (expr, 0) = get_initialized_tmp_var (op0, pre_p,
> +							      post_p);

How does this follow?  I can't think of any valid case for
which we have a conditional store to a formal temporary.


r~

Re: [PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On 3/20/07, Richard Henderson <rth@redhat.com> wrote:
> On Wed, Mar 14, 2007 at 02:58:28PM -0800, Andrew_Pinski@PlayStation.Sony.Com wrote:
> > +       /* If we don't already have an addressable expression, create a new
> > +          decl to hold it so we don't get non gimple as the decl which was
> > +          holding this before was not marked as addressable already.  */
> > +       if (TREE_CODE (op0) == VAR_DECL
> > +           && DECL_GIMPLE_FORMAL_TEMP_P (op0)
> > +           && !TREE_ADDRESSABLE (op0))
> > +         TREE_OPERAND (expr, 0) = get_initialized_tmp_var (op0, pre_p,
> > +                                                           post_p);
>
> How does this follow?  I can't think of any valid case for
> which we have a conditional store to a formal temporary.

I removed this part since it was not important to fix the testcase (I
will revisit this for another bug).

OK? Bootstrapped and tested on i686-apple-darwin.

Thanks,
Andrew Pinski


ChangeLog:

       * gimplifier.c (gimplify_cond_expr): Don't create *(a?&b:&c), if a
       rvalue is ok.

cp/ChangeLog:

       * typeck.c (build_address): For COND_EXPR, create a?&b:&c instead
       of the plain &(a?b:c) so we don't get a temp variable in the gimplifier.

testsuite/ChangeLog:

       * gcc.c-torture/compile/complex-5.c: New test.

[-- Attachment #2: fixpr30132.diff.txt --]
[-- Type: text/plain, Size: 1454 bytes --]

Index: testsuite/gcc.c-torture/compile/complex-5.c
===================================================================
--- testsuite/gcc.c-torture/compile/complex-5.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/complex-5.c	(revision 0)
@@ -0,0 +1,13 @@
+/* PR 30132 */
+#define complex _Complex
+void testit(double complex* t, double* b)
+{
+  b[0] = t[0]==0.0?0.0:-t[0];
+}
+
+main(void)
+{
+  static double complex k = 5;
+  static double b;
+  testit(&k,&b);
+}
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 129843)
+++ cp/typeck.c	(working copy)
@@ -4082,6 +4082,14 @@
   if (error_operand_p (t) || !cxx_mark_addressable (t))
     return error_mark_node;
 
+  if (TREE_CODE (t) == COND_EXPR)
+    {
+      tree ptrtype = build_pointer_type (TREE_TYPE (t));
+      return build3 (COND_EXPR, ptrtype, TREE_OPERAND (t, 0),
+		     build_address (TREE_OPERAND (t, 1)),
+		     build_address (TREE_OPERAND (t, 2)));
+    }
+
   addr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (t)), t);
 
   return addr;
Index: gimplify.c
===================================================================
--- gimplify.c	(revision 129843)
+++ gimplify.c	(working copy)
@@ -2577,7 +2577,7 @@
     {
       tree result;
 
-      if ((fallback & fb_lvalue) == 0)
+      if (fallback != fb_lvalue)
 	{
 	  result = tmp2 = tmp = create_tmp_var (TREE_TYPE (expr), "iftmp");
 	  ret = GS_ALL_DONE;

Re: [PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[Richard Guenther]

On 11/2/07, Andrew Pinski <pinskia@gmail.com> wrote:
> On 3/20/07, Richard Henderson <rth@redhat.com> wrote:
> > On Wed, Mar 14, 2007 at 02:58:28PM -0800, Andrew_Pinski@PlayStation.Sony.Com wrote:
> > > +       /* If we don't already have an addressable expression, create a new
> > > +          decl to hold it so we don't get non gimple as the decl which was
> > > +          holding this before was not marked as addressable already.  */
> > > +       if (TREE_CODE (op0) == VAR_DECL
> > > +           && DECL_GIMPLE_FORMAL_TEMP_P (op0)
> > > +           && !TREE_ADDRESSABLE (op0))
> > > +         TREE_OPERAND (expr, 0) = get_initialized_tmp_var (op0, pre_p,
> > > +                                                           post_p);
> >
> > How does this follow?  I can't think of any valid case for
> > which we have a conditional store to a formal temporary.
>
> I removed this part since it was not important to fix the testcase (I
> will revisit this for another bug).
>
> OK? Bootstrapped and tested on i686-apple-darwin.

The middle-end parts are ok.

Thanks,
Richard.

> Thanks,
> Andrew Pinski
>
>
> ChangeLog:
>
>        * gimplifier.c (gimplify_cond_expr): Don't create *(a?&b:&c), if a
>        rvalue is ok.
>
> cp/ChangeLog:
>
>        * typeck.c (build_address): For COND_EXPR, create a?&b:&c instead
>        of the plain &(a?b:c) so we don't get a temp variable in the gimplifier.
>
> testsuite/ChangeLog:
>
>        * gcc.c-torture/compile/complex-5.c: New test.
>
>

Re: [PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[Jakub Jelinek]

On Fri, Nov 02, 2007 at 03:08:19PM +0100, Richard Guenther wrote:
> > OK? Bootstrapped and tested on i686-apple-darwin.
> 
> The middle-end parts are ok.

> >        * gimplifier.c (gimplify_cond_expr): Don't create *(a?&b:&c), if a
> >        rvalue is ok.

Wouldn't it be better instead if fallback is fb_either check whether the
COND_EXPR can be an lvalue or not and only take addresses of both branches
if it can be an lvalue?  is_gimple_addressable on both branches wouldn't
be probably enough, we'd need to also check recursively for further
COND_EXPRs.  On the testcase in this PR, one side is COMPLEX_CST, another is
NEGATE_EXPR, so obviously neither can be an lvalue...

And/or reconsider
  /* Step 2 is to gimplify the base expression.  Make sure lvalue is set
     so as to match the min_lval predicate.  Failure to do so may result
     in the creation of large aggregate temporaries.  */
  tret = gimplify_expr (p, pre_p, post_p, is_gimple_min_lval,
                        fallback | fb_lvalue);
if p is an scalar or complex.

	Jakub

Re: [PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[Jason Merrill]

Andrew Pinski wrote:
>        * typeck.c (build_address): For COND_EXPR, create a?&b:&c instead
>        of the plain &(a?b:c) so we don't get a temp variable in the gimplifier.

What about the other transformations in unary_complex_lvalue?  Or the &* 
optimization?

The problem is that build_address is trying to do something very simple, 
but the name makes it attractive to other parts of the compiler that 
want the address of something without all the diagnostics in 
build_unary_op.  Really it's only safe to call build_address if you know 
that there aren't any tree simplifications to be done on the operand.

I think what we really need is to split out most of the address handling 
in build_unary_op so we can call it with diagnostics on or off, call the 
latter build_address, rename the current build_address to 
build_addr_expr and go through the callers of build_address to see which 
one they really want.

Jason

Re: [PATCH] Fix middle-end/30132: ICE with complex and taking the real part of a ?:

[Jason Merrill]

Andrew Pinski wrote:
>        * typeck.c (build_address): For COND_EXPR, create a?&b:&c instead
>        of the plain &(a?b:c) so we don't get a temp variable in the gimplifier.

What about the other transformations in unary_complex_lvalue?  Or the &* 
optimization?

The problem is that build_address is trying to do something very simple, 
but the name makes it attractive to other parts of the compiler that 
want the address of something without all the diagnostics in 
build_unary_op.  Really it's only safe to call build_address if you know 
that there aren't any tree simplifications to be done on the operand.

I think what we really need is to split out most of the address handling 
in build_unary_op so we can call it with diagnostics on or off, call the 
latter build_address, rename the current build_address to 
build_addr_expr and go through the callers of build_address to see which 
one they really want.

Jason