Fix processing of ADDR_EXPR in get_expr_operands

Fix processing of ADDR_EXPR in get_expr_operands

[Diego Novillo]

As discussed yesterday, is_gimple_min_invariant is the correct test for
checking constness in an ADDR_EXPR node.

Bootstrapped and tested x86, x86-64, alpha and ppc.


Diego.

	* tree-ssa-operands.c (get_expr_operands): Revert changes
	to ADDR_EXPR processing introduced by:
	    2004-06-21  Richard Kenner  <kenner@vlsi1.ultra.nyu.edu>
	    * tree-ssa-operands.c (get_expr_operands): Minor rearrangements.


testsuite/ChangeLog

	* gcc.dg/tree-ssa/20040729-1.c: New test case.

Index: tree-ssa-operands.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-ssa-operands.c,v
retrieving revision 2.26
diff -d -c -p -r2.26 tree-ssa-operands.c
*** tree-ssa-operands.c	28 Jul 2004 05:13:08 -0000	2.26
--- tree-ssa-operands.c	29 Jul 2004 17:07:51 -0000
*************** get_expr_operands (tree stmt, tree *expr
*** 859,867 ****
  	 of interest to some passes (e.g. alias resolution).  */
        add_stmt_operand (expr_p, stmt, 0, NULL);
  
!       /* If the address is constant (invariant is not sufficient), there will
! 	 be no interesting variable references inside.  */
!       if (TREE_CONSTANT (expr))
  	return;
  
        /* There should be no VUSEs created, since the referenced objects are
--- 859,867 ----
  	 of interest to some passes (e.g. alias resolution).  */
        add_stmt_operand (expr_p, stmt, 0, NULL);
  
!       /* If the address is invariant, there may be no interesting variable
! 	 references inside.  */
!       if (is_gimple_min_invariant (expr))
  	return;
  
        /* There should be no VUSEs created, since the referenced objects are
Index: testsuite/gcc.dg/tree-ssa/20040729-1.c
===================================================================
RCS file: testsuite/gcc.dg/tree-ssa/20040729-1.c
diff -N testsuite/gcc.dg/tree-ssa/20040729-1.c
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gcc.dg/tree-ssa/20040729-1.c	29 Jul 2004 17:07:55 -0000
***************
*** 0 ****
--- 1,18 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O1 -fdump-tree-dce3" } */
+ 
+ foo ()
+ {
+   volatile int *p;
+   volatile int x;
+ 
+   p = &x;
+   *p = 3;
+   return *p + 1;
+ }
+ 
+ /* The assignment to 'p' is dead and should be removed.  But the
+    compiler was mistakenly thinking that the statement had volatile
+    operands.  But 'p' itself is not volatile and taking the address of
+    a volatile does not constitute a volatile operand.  */
+ /* { dg-final { scan-tree-dump-times "&x" 0 "dce3"} } */

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Richard Kenner]

    As discussed yesterday, is_gimple_min_invariant is the correct test for
    checking constness in an ADDR_EXPR node.

I still haven't seen an answer to why we're not marking addresses of
volatile as constant, which I think is the real bug here.

It's possible the bug my original change fixed is no longer relevant, but
if this breaks something, I'll let you know.

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Diego Novillo]

On Thu, 2004-07-29 at 21:56, Richard Kenner wrote:

> I still haven't seen an answer to why we're not marking addresses of
> volatile as constant, which I think is the real bug here.
> 
Invariant addresses are supposed to be marked with TREE_INVARIANT, not
TREE_CONSTANT.  That's what I've been told by the FE folks and that's
why the tree optimizers almost always rely on is_gimple_min_invariant().

/* Value of expression is function invariant.  A strict subset of
   TREE_CONSTANT, such an expression is constant over any one function
   invocation, though not across different invocations.  May appear in
   any expression node.  */
#define TREE_INVARIANT(NODE) ((NODE)->common.invariant_flag)
                                                                                
> It's possible the bug my original change fixed is no longer relevant, but
> if this breaks something, I'll let you know.
>
Make your FE set TREE_INVARIANT on "constant" ADDR_EXPRs and it will all
work fine in get_expr_operands.


Diego.

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Richard Henderson]

On Thu, Jul 29, 2004 at 09:56:20PM -0400, Richard Kenner wrote:
> I still haven't seen an answer to why we're not marking addresses of
> volatile as constant, which I think is the real bug here.

I *told* you.  The variable is *local*, and thus TREE_INVARIANT
is the correct bit to set, not TREE_CONSTANT.


r~

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Richard Kenner]

    > I still haven't seen an answer to why we're not marking addresses of
    > volatile as constant, which I think is the real bug here.

    Invariant addresses are supposed to be marked with TREE_INVARIANT, not
    TREE_CONSTANT.  That's what I've been told by the FE folks and that's
    why the tree optimizers almost always rely on
    is_gimple_min_invariant().

What does this have to do with the volatile question I'm asking about?

    Make your FE set TREE_INVARIANT on "constant" ADDR_EXPRs and it will all
    work fine in get_expr_operands.

To repeat what I said before, it's not the job of a front end to set
these flags on ADDR_EXPRs and they would be blown away by the middle-end
if it did.  They are set by recompute_tree_invarant_for_addr_expr.

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Diego Novillo]

On Thu, 2004-07-29 at 22:38, Richard Kenner wrote:

>     Invariant addresses are supposed to be marked with TREE_INVARIANT, not
>     TREE_CONSTANT.  That's what I've been told by the FE folks and that's
>     why the tree optimizers almost always rely on
>     is_gimple_min_invariant().
> 
> What does this have to do with the volatile question I'm asking about?
> 
It's rather obvious.  You cannot rely on TREE_CONSTANT to determine
constness of ADDR_EXPRs.

I don't know *why* ADDR_EXPR of a volatile is not TREE_CONSTANT.  I
don't really care.  All I know, and always use, is that constness of
ADDR_EXPRs is tested with TREE_INVARIANT.  Hence the call to
is_gimple_min_invariant.  Even TREE_CONSTANT is not always right, you
have to check for overflow as well.

> these flags on ADDR_EXPRs and they would be blown away by the middle-end
> if it did.  They are set by recompute_tree_invarant_for_addr_expr.
>
Which you will notice sets both TREE_CONSTANT and TREE_INVARIANT, not
necessarily with the same value.


Diego.

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Richard Henderson]

On Thu, Jul 29, 2004 at 10:38:53PM -0400, Richard Kenner wrote:
> What does this have to do with the volatile question I'm asking about?

Not a thing.  But it turns out that volatile isn't relevant either.


r~

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Richard Kenner]

    It's rather obvious.  You cannot rely on TREE_CONSTANT to determine
    constness of ADDR_EXPRs.

You lost me.  I thought we took a lot of care to make sure it *does*
determine precisely that?

    I don't know *why* ADDR_EXPR of a volatile is not TREE_CONSTANT.  I
    don't really care.  

Well, I do.  It sounds like a bug to me.  Indeed it sounds like the central
bug in what you're trying to fix.

    All I know, and always use, is that constness of ADDR_EXPRs is tested
    with TREE_INVARIANT.  Hence the call to is_gimple_min_invariant.  Even
    TREE_CONSTANT is not always right, you have to check for overflow as
    well.

Overflow?

    Which you will notice sets both TREE_CONSTANT and TREE_INVARIANT, not
    necessarily with the same value.

Right, I know it does.  Indeed I made some changes to that code.

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Diego Novillo]

On Thu, 2004-07-29 at 23:27, Richard Kenner wrote:

>     I don't know *why* ADDR_EXPR of a volatile is not TREE_CONSTANT.  I
>     don't really care.  
> 
> Well, I do.  It sounds like a bug to me.  Indeed it sounds like the central
> bug in what you're trying to fix.
> 
No.  It is not a bug.  I don't know how else to tell you the same
thing.  Address constness is checked with TREE_INVARIANT.  The address
of a variable may be invariant in one invocation to a function, but have
different values from call to call.

>     All I know, and always use, is that constness of ADDR_EXPRs is tested
>     with TREE_INVARIANT.  Hence the call to is_gimple_min_invariant.  Even
>     TREE_CONSTANT is not always right, you have to check for overflow as
>     well.
> 
> Overflow?
> 
Yes.  Overflow.

>     Which you will notice sets both TREE_CONSTANT and TREE_INVARIANT, not
>     necessarily with the same value.
> 
> Right, I know it does.  Indeed I made some changes to that code.
>
Then why are we having this conversation?  You should know why we set
TREE_INVARIANT instead of TREE_CONSTANT, then.


Diego.

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Richard Kenner]

    I *told* you.  The variable is *local*, and thus TREE_INVARIANT
    is the correct bit to set, not TREE_CONSTANT.

Oh, sorry.  I've never heard of a use for a volatile *local* variable, so
I just assumed it was global and hence that TREE_CONSTANT would apply.

I'll do a bootstrap soon and see if the motivation for using TREE_CONSTANT
instead of TREE_INVARIANT there is still present.

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Richard Kenner]

    No.  It is not a bug.  I don't know how else to tell you the same
    thing.  Address constness is checked with TREE_INVARIANT.  The address
    of a variable may be invariant in one invocation to a function, but have
    different values from call to call.

The two mean two different thing: "invariant" means it won't change during
the execution of the function while "constant" means it won't change *at all*.
I can easily see cases where the first is what's wanted and where the second
is what's wanted.  It may well be that in that particular case, "invariant"
is the wanted property, but I don't accept that that's *always* the case.

    Yes.  Overflow.

Why do we care about overflow?  If an addressing computation overflows, it's
still a constant!

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Diego Novillo]

On Fri, 2004-07-30 at 09:27, Richard Kenner wrote:
>     No.  It is not a bug.  I don't know how else to tell you the same
>     thing.  Address constness is checked with TREE_INVARIANT.  The address
>     of a variable may be invariant in one invocation to a function, but have
>     different values from call to call.
> 
> The two mean two different thing: "invariant" means it won't change during
> the execution of the function while "constant" means it won't change *at all*.
>
I feel like I'm talking to myself here.  Yes, that's exactly what it
means and that's exactly what I've been saying all this time.

> I can easily see cases where the first is what's wanted and where the second
> is what's wanted.  It may well be that in that particular case, "invariant"
> is the wanted property, but I don't accept that that's *always* the case.
> 
For address constants?  Yes, that's always the case.  CONSTANT is a
superset of INVARIANT.  In an intra-procedural setting, we care for
invariant addresses, we don't care if they are constant or not.

>     Yes.  Overflow.
> 
> Why do we care about overflow?  If an addressing computation overflows, it's
> still a constant!
>
No.  That's for real constants.  I was describing what
is_gimple_min_invariant does.  Check the code out.


Diego.

Re: Fix processing of ADDR_EXPR in get_expr_operands

[Richard Kenner]

    I feel like I'm talking to myself here.  Yes, that's exactly what it
    means and that's exactly what I've been saying all this time.

It sounds like we're in what's often called "violent agreement", but saying
the same thing different ways.

    For address constants?  Yes, that's always the case.  CONSTANT is a
    superset of INVARIANT.  In an intra-procedural setting, we care for
    invariant addresses, we don't care if they are constant or not.

A few things:

(1) I'm not at all sure if the handling of nested variables is correct
or even what it should be.

(2) I'm not as convinced as you are that there is no conceivable
reason why constant might not be the proper check within a function.
But I indeed can't think of any at the moment.

(3) I think I may know what motivated the change I made: the stupid VLA
case we've been discussing.  There you have a case of a decl in the current
function whose address is *not* invariant.  And I think there are (or at
least used to be) other variables whose DECL_RTL is a MEM of a pseudo.
In all of those cases, it isn't invariant either: the invariance is
over the life of the variable, but not over the function.