Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Richard Kenner]

    Because the COMPONENT_REF has its TREE_SIDE_EFFECTS set then by
    recompute_tree_invarant_for_addr_expr so does the ADDR_EXPR, this
    might be a bug in that function though.

I think it is.  But the TREE_SIDE_EFFECTS bit in the COMPONENT_REF is
ignored; the issue is that the operand of the COMPONENT_REF also must have
TREE_SIDE_EFFECTS set.  But I agree that shouldn't set TREE_SIDE_EFFECTS of
an ADDR_EXPR: only if the computation of the *address* has side-effects should
TREE_SIDE_EFFECTS be set.

Specifically, I think the line

      se |= TREE_SIDE_EFFECTS (node);

should be deleted.

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Jason Merrill]

On Mon, 18 Oct 04 15:40:57 EDT, kenner@vlsi1.ultra.nyu.edu (Richard Kenner) wrote:

>     Because the COMPONENT_REF has its TREE_SIDE_EFFECTS set then by
>     recompute_tree_invarant_for_addr_expr so does the ADDR_EXPR, this
>     might be a bug in that function though.
>
> I think it is.  But the TREE_SIDE_EFFECTS bit in the COMPONENT_REF is
> ignored; the issue is that the operand of the COMPONENT_REF also must have
> TREE_SIDE_EFFECTS set.  But I agree that shouldn't set TREE_SIDE_EFFECTS of
> an ADDR_EXPR: only if the computation of the *address* has side-effects should
> TREE_SIDE_EFFECTS be set.
>
> Specifically, I think the line
>
>       se |= TREE_SIDE_EFFECTS (node);
>
> should be deleted.

I think that should still be the general case; if we're taking the address
of, say, a struct returned from a function, we want to propagate
TREE_SIDE_EFFECTS from the call to the ADDR_EXPR.

But we should special-case COMPONENT_REF so that TREE_SIDE_EFFECTS is
propagated from op0 to the ADDR_EXPR, not from the ref itself.  With
appropriate recursion.

Jason

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Richard Kenner]

    Yes but we don't UPDATE_TITCSE on the INDRIECT_REF for the case which I 
    am looking at.

    We have <COMPONENT_REF <INDIRECT_REF <VAR_DECL <VOLATILE> SIDE_EFFECTS>
    SIDE_EFFECTS> in my case.  

That's *exactly* the case I'm talking about!  But in this case since
the VAR_DECL is volatile, the ADDR_EXPR *should* have
TREE_SIDE_EFFECTS since it's basically the volatile var plus a
constant.  But I still think the code should be looking at the operand
of the INDIRECT_REF (the result would be the same, though).

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Richard Kenner]

    I think that should still be the general case; if we're taking the address
    of, say, a struct returned from a function, we want to propagate
    TREE_SIDE_EFFECTS from the call to the ADDR_EXPR.

I wonder if the problem isn't here:

  if (TREE_CODE (node) == INDIRECT_REF)
    {
      /* If this is &((T*)0)->field, then this is a form of addition.  */
      if (TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST)
	UPDATE_TITCSE (node);

Shouldn't that last "node" be TREE_OPERAND (node, 0)?  We don't care about
the INDIRECT_REF here: all we care about is its operand.

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Andrew Pinski]

On Oct 18, 2004, at 3:58 PM, Richard Kenner wrote:

>     I think that should still be the general case; if we're taking the 
> address
>     of, say, a struct returned from a function, we want to propagate
>     TREE_SIDE_EFFECTS from the call to the ADDR_EXPR.
>
> I wonder if the problem isn't here:
>
>   if (TREE_CODE (node) == INDIRECT_REF)
>     {
>       /* If this is &((T*)0)->field, then this is a form of addition.  
> */
>       if (TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST)
> 	UPDATE_TITCSE (node);
>
> Shouldn't that last "node" be TREE_OPERAND (node, 0)?  We don't care 
> about
> the INDIRECT_REF here: all we care about is its operand.

Yes but we don't UPDATE_TITCSE on the INDRIECT_REF for the case which I 
am
looking at.
We have <COMPONENT_REF <INDIRECT_REF <VAR_DECL <VOLATILE> SIDE_EFFECTS>
SIDE_EFFECTS> in my case.  The reason why COMPONENT_REF got the 
SIDE_EFFECTS
is because the INDIRECT_REF got it, this is done in build3. So the 
address
expression still gets the SIDE_EFFECTS.

Here is another testcase where SIDE_EFFECTS needs to be set to true and
where we ICE without the patch:
struct lock {
  int lk_interlock;
};
volatile struct lock *f();
void
acquire(void)
{
   &f()->lk_interlock;
}

Maybe we should ignore ADDR_EXPR instead of the patch where I call
gimplify_addr_expr (but I could be wrong).

-- Pinski

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Richard Kenner]

    I think that should still be the general case; if we're taking the address
    of, say, a struct returned from a function, we want to propagate
    TREE_SIDE_EFFECTS from the call to the ADDR_EXPR.

Oh, you're right.  I misread the code.  It only does that if the innermost
object is not a decl.  So I agree it should be there.

    But we should special-case COMPONENT_REF so that TREE_SIDE_EFFECTS is
    propagated from op0 to the ADDR_EXPR, not from the ref itself.  With
    appropriate recursion.

But that's what the code does: it ignores the TREE_SIDE_EFFECTS of the
COMPONENT_REF!

[PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Andrew Pinski]

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

The problem here is that we don't handle taking the address of
volatile pointers to structs' members, aka we don't handle
when we have volatile types but the expression it self is not
volatile. This fixes the problem by handling them.

OK? Bootstrapped and tested on powerpc-darwin with no regressions.

Thanks,
Andrew Pinski

ChangeLog:
	* gimplify.c (gimplify_expr): Handle ADDR_EXPR when
	we have volatile types but the expression it self is
	not volatile.

testsuite/ChangeLog:
	* gcc.dg/pr17885.c: New test.


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

Index: testsuite/gcc.dg/pr17885.c
===================================================================
RCS file: testsuite/gcc.dg/pr17885.c
diff -N testsuite/gcc.dg/pr17885.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gcc.dg/pr17885.c	18 Oct 2004 14:24:30 -0000
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* We used to ICE in the gimplifier because
+   we have volatile types but the ADDR_EXPR 
+   it self is not volatile.  */
+
+struct lock {
+ int lk_interlock;
+};
+void
+acquire(volatile struct lock *lkp)
+{
+  &lkp->lk_interlock;
+}
Index: gimplify.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gimplify.c,v
retrieving revision 2.83
diff -u -p -r2.83 gimplify.c
--- gimplify.c	11 Oct 2004 12:57:09 -0000	2.83
+++ gimplify.c	18 Oct 2004 14:24:31 -0000
@@ -3973,6 +3995,10 @@ gimplify_expr (tree *expr_p, tree *pre_p
 			     gimple_test_f, fallback);
 	      break;
 
+	    case ADDR_EXPR:
+	      gimplify_addr_expr (expr_p, pre_p, post_p);
+	      break;
+
 	    default:
 	       /* Anything else with side-effects must be converted to
 	       	  a valid statement before we get here.  */

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Richard Henderson]

On Mon, Oct 18, 2004 at 10:24:45AM -0400, Andrew Pinski wrote:
> 	* gimplify.c (gimplify_expr): Handle ADDR_EXPR when
> 	we have volatile types but the expression it self is
> 	not volatile.

Without further information, I think this is wrong.  Why
in the world is TREE_SIDE_EFFECTS for such an ADDR_EXPR?


r~

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Andrew Pinski]

On Oct 18, 2004, at 1:54 PM, Richard Henderson wrote:

> On Mon, Oct 18, 2004 at 10:24:45AM -0400, Andrew Pinski wrote:
>> 	* gimplify.c (gimplify_expr): Handle ADDR_EXPR when
>> 	we have volatile types but the expression it self is
>> 	not volatile.
>
> Without further information, I think this is wrong.  Why
> in the world is TREE_SIDE_EFFECTS for such an ADDR_EXPR?

Because the COMPONENT_REF has its TREE_SIDE_EFFECTS set
then by recompute_tree_invarant_for_addr_expr
so does the ADDR_EXPR, this might be a bug in
that function though.

Thanks,
Andrew Pinski

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Richard Henderson]

On Mon, Oct 18, 2004 at 02:17:23PM -0400, Andrew Pinski wrote:
> Because the COMPONENT_REF has its TREE_SIDE_EFFECTS set
> then by recompute_tree_invarant_for_addr_expr
> so does the ADDR_EXPR, this might be a bug in
> that function though.

Indeed it is.  I'll look into it.


r~

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Richard Henderson]

On Mon, Oct 18, 2004 at 02:17:23PM -0400, Andrew Pinski wrote:
> ... recompute_tree_invarant_for_addr_expr so does the ADDR_EXPR,
> this might be a bug in that function though.

I'm currently testing the following patch.


r~


Index: tree.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree.c,v
retrieving revision 1.437
diff -c -p -d -r1.437 tree.c
*** tree.c	29 Sep 2004 13:07:44 -0000	1.437
--- tree.c	18 Oct 2004 21:34:25 -0000
*************** do { tree _node = (NODE); \
*** 2311,2326 ****
      }
  
    /* Now see what's inside.  If it's an INDIRECT_REF, copy our properties from
!      it.  If it's a decl, it's invariant and constant if the decl is static.
!      It's also invariant if it's a decl in the current function.  (Taking the
!      address of a volatile variable is not volatile.)  If it's a constant,
!      the address is both invariant and constant.  Otherwise it's neither.  */
    if (TREE_CODE (node) == INDIRECT_REF)
!     {
!       /* If this is &((T*)0)->field, then this is a form of addition.  */
!       if (TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST)
! 	UPDATE_TITCSE (node);
!     }
    else if (DECL_P (node))
      {
        if (staticp (node))
--- 2311,2323 ----
      }
  
    /* Now see what's inside.  If it's an INDIRECT_REF, copy our properties from
!      the address, since &(*a)->b is a form of addition.  If it's a decl, it's
!      invariant and constant if the decl is static.  It's also invariant if it's
!      a decl in the current function.  Taking the address of a volatile variable
!      is not volatile.  If it's a constant, the address is both invariant and
!      constant.  Otherwise it's neither.  */
    if (TREE_CODE (node) == INDIRECT_REF)
!     UPDATE_TITCSE (TREE_OPERAND (node, 0));
    else if (DECL_P (node))
      {
        if (staticp (node))

Re: [PATCH] fix PR middle-end/17885, gimplifing of volatile &a->b

[Jeffrey A Law]

On Mon, 2004-10-18 at 08:24, Andrew Pinski wrote:
> The problem here is that we don't handle taking the address of
> volatile pointers to structs' members, aka we don't handle
> when we have volatile types but the expression it self is not
> volatile. This fixes the problem by handling them.
> 
> OK? Bootstrapped and tested on powerpc-darwin with no regressions.
> 
> Thanks,
> Andrew Pinski
> 
> ChangeLog:
> 	* gimplify.c (gimplify_expr): Handle ADDR_EXPR when
> 	we have volatile types but the expression it self is
> 	not volatile.
So can you explain a little more about how this patch actually
fixes the problem?  How does adding a second call to
gimplify_addr_expr change anything?   All you've done is avoid
the default case which aborts -- you haven't (as far as I can tell)
changed the gimple code we produce all.

It seems to me you really wanted to call gimplify_expr to remove
the embedded volatile reference.

jeff