Re: MEM/u trouble

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Mar 13, 2001 at 10:50:22AM -0500, Richard Kenner wrote:
>     I thought that (mem (addressof ())) means a stack slot has not been
>     assigned for it yet and it is unclear whether it is actually needed,
> 
> Only if what's inside the ADDRESSOF is a REG, not a MEM.

Oopsie, I meant (mem (addressof (reg))).

> 
>     so IMHO clearing the /u flag should just kill it for all mems for that
>     addressof and nothing else.
> 
> No, because the stack slot will be created from the type of the variable,
> and so the /u will be determined that way.

If (mem (addressof (reg))) is converted to (mem ()), then AFAIC the MEM
flags are preserved.

> 
>     In
>       foo(const K x) : h(x) {}
>     x is a const variable (so its VAR_DECLs DECL_RTL got /u) and is addressable
>     (so it was put_var_into_stack() and got assigned a
>     (addressof:SI (reg/v/u:SI 63) 62 0x401d5888)).
> 
>     x is being initialized from a temporary variable (surrounded by
>     TARGET_EXPR) and expand_expr in the TARGET_EXPR case decides it can
>     optimize things by simply initializing the temporary variable at the
>     location of x (and thus avoid copying). But the temporary variable is
>     not const whyle x is.
> 
> I'm sorry: I still don't follow.  It looks like TARGET_EXPR looks at DECL_RTL,
> which should have been set properly.  Remember that I don't know C++, so
> you have to explain this just in terms of the defined tree and RTL.

I'll try once more:

struct J { int c; J(int j) : c(j) {} };
struct K { int d; K(int k) : d(k) {} K(const J &l) : d(l.c) {} };
J u();
struct foo { K h; foo(const K m) : h(m) {} };
foo *f;
void bar() { f = new foo(u()); }

The m parameter of foo constructor is readonly (because of the const
qualifier):

 <var_decl 0x401d5af8 m
    type <record_type 0x401cd138 K readonly needs-constructing type_1 type_5 SI
        size <integer_cst 0x401a2be0 constant 32>
        unit size <integer_cst 0x401a2c00 constant 4>
        user align 32 symtab 0 alias set -1
        fields <field_decl 0x401ccc30 d type <integer_type 0x401a6340 int>
            nonlocal decl_3 SI file u.C line 2
            size <integer_cst 0x401a25e0 constant 32>
            unit size <integer_cst 0x401a2600 constant 4>
            align 32 offset_align 128
            offset <integer_cst 0x401a2cc0 constant 0>
            bit offset <integer_cst 0x401a2ce0 constant 0> context <record_type 0x401cca90 K> arguments <integer_cst 0x401a2cc0 0> chain <type_decl 0x401ccbc8 K>>
       needs-constructor X(constX&) this=(X&) n_parents 0 use_template=0 interface-unknown vtable-needs-writing
        member-functions <tree_vec 0x401cb700
            elt 0 <overload 0x401c9d60>
            elt 2 <function_decl 0x401cd680 operator=>
            elt 3 <overload 0x401c9e20>
            elt 4 <overload 0x401c9e00>>
        pointer_to_this <pointer_type 0x401d1a28> reference_to_this <reference_type 0x401cd1a0>>
    readonly used SI file u.C line 4 size <integer_cst 0x401a2be0 32> unit size <integer_cst 0x401a2c00 4>
    user align 32 context <function_decl 0x401d5138 bar> abstract_origin <parm_decl 0x401cfc30 m>
    (reg/v/u:SI 62)>

(but if put_var_into_stack has been called on it as it happens in the slightly larger testcases, it has
(mem/v/u:SI (addressof:SI (reg/v/u:SI 63) ...) style DECL_RTL).
I don't see anything wrong with m being readonly at this time.
Later on, there is expand_assignment called with this 0x401d5af8 as to and
 <target_expr 0x401d44e0
    type <record_type 0x401cca90 K needs-constructing type_1 type_5 SI
        size <integer_cst 0x401a2be0 constant 32>
        unit size <integer_cst 0x401a2c00 constant 4>
        user align 32 symtab 0 alias set 0
        fields <field_decl 0x401ccc30 d type <integer_type 0x401a6340 int>
            nonlocal decl_3 SI file u.C line 2
            size <integer_cst 0x401a25e0 constant 32>
            unit size <integer_cst 0x401a2600 constant 4>
            align 32 offset_align 128
            offset <integer_cst 0x401a2cc0 constant 0>
            bit offset <integer_cst 0x401a2ce0 constant 0> context <record_type 0x401cca90 K> arguments <integer_cst 0x401a2cc0 0> chain <type_decl 0x401ccbc8 K>>
       needs-constructor X(constX&) this=(X&) n_parents 0 use_template=0 interface-unknown vtable-needs-writing
        member-functions <tree_vec 0x401cb700
            elt 0 <overload 0x401c9d60>
            elt 2 <function_decl 0x401cd680 operator=>
            elt 3 <overload 0x401c9e20>
            elt 4 <overload 0x401c9e00>>
        pointer_to_this <pointer_type 0x401ccaf8> reference_to_this <reference_type 0x401cd478> chain <type_decl 0x401ccb60 K>>
    side-effects addressable
    arg 0 <var_decl 0x401d5888 type <record_type 0x401cca90 K>
        addressable used SI file u.C line 6 size <integer_cst 0x401a2be0 32> unit size <integer_cst 0x401a2c00 4>
        user align 32 context <function_decl 0x401d5138 bar>
        (mem/s:SI (addressof:SI (reg/v:SI 63) 62 0x401d5888) 0)>
    arg 1 <compound_expr 0x401d4780 type <record_type 0x401cca90 K>
        side-effects
        arg 0 <expr_with_file_location 0x401d4860 type <void_type 0x401a97b8 void>
            side-effects used public
            arg 0 <stmt_expr 0x401d6460 type <void_type 0x401a97b8 void>
                side-effects
                arg 0 <scope_stmt 0x401d649c tree_0 tree_3
                    arg 0 <block 0x401d47e0 used vars <var_decl 0x401d5b60 this> abstract_origin <function_decl 0x401cd068 K>>
                    chain <decl_stmt 0x401d6474 arg 0 <var_decl 0x401d5b60 this>
                        chain <decl_stmt 0x401d6488 arg 0 <var_decl 0x401d5bc8 l> chain <scope_stmt 0x401d64b0>>>>>
            arg 1 <identifier_node 0x401d0600 u.C>
            u.C:2:0> arg 1 <var_decl 0x401d5888>>>
as from. This surrounds a temporary variable 0x401d5888, which is constructed
using the K(const J &l) constructor from the u() return value. This
temporary variable is not const. expand_assignment calls store_expr with
0x401d44e0 as the first argument and DECL_RTL of the `m' variable as target.
The [TARGET_EXPR] case in expand_expr instead of assigning a stack slot for
0x401d5888 and returning it in there decides to construct it already in the
target RTL, but as the target RTL is RTX_UNCHANGING_P and the construction
goes to it via 0x401d5888's trees (ie. non-const), we may end up with rtl
where a memory slot is initialized using non-RTX_UNCHANGING_P accesses and
read using RTX_UNCHANGING_P accesses which as Geoff said is not allowed (and
in the particular testcase I had some compiler snapshots actually reordered
the result so that initialization was done after loads from that location).
I think it should be expand_expr's responsibility to handle this, since
requiring each frontend to find out first what will be constant variables
initialized from and clearing their constant flags does not look right to
me.

Attached are 3 expand_expr patches. P1 only clears the /u flag if target is
(reg/u) or (mem/u (addressof (reg))), P2 forces creation of temporary
variable (safe, but inefficient) and P3 forces creation of temporary
variable if target is not (reg/u) or (mem/u (addressof (reg))), otherwise
clears the /u flag (which is IMHO usually cheaper than keeping the /u flag
and creating a temporary variable and copying it to the const location).

	Jakub
--- expr.c.jj	Tue Mar 13 14:04:14 2001
+++ expr.c	Wed Mar 14 19:31:44 2001
@@ -8329,6 +8329,17 @@ expand_expr (exp, target, tmode, modifie
 	    else
 	      {
 		DECL_RTL (slot) = target;
+
+		/* If target is unchanging, but slot is not, we could end up
+		   initializing the unchanging target through non-unchanging
+		   references.  */
+		if (RTX_UNCHANGING_P (target) && ! TREE_READONLY (slot)
+		    && (GET_CODE (target) == REG
+			|| (GET_CODE (target) == MEM
+			    && GET_CODE (XEXP (target, 0)) == ADDRESSOF
+			    && GET_CODE (XEXP (XEXP (target, 0), 0)) == REG)))
+		  RTX_UNCHANGING_P (target) = 0;
+
 		/* If we must have an addressable slot, then make sure that
 		   the RTL that we just stored in slot is OK.  */
 		if (TREE_ADDRESSABLE (slot))
--- expr.c.jj	Tue Mar 13 14:04:14 2001
+++ expr.c	Wed Mar 14 19:27:58 2001
@@ -8277,6 +8277,40 @@ expand_expr (exp, target, tmode, modifie
 	   knows that it should fix up those uses.  */
 	TREE_USED (slot) = 1;
 
+	if (target)
+	  {
+	    /* This case does occur, when expanding a parameter which
+	       needs to be constructed on the stack.  The target
+	       is the actual stack address that we want to initialize.
+	       The function we call will perform the cleanup in this case.  */
+
+	    /* If we have already assigned it space, use that space,
+	       not target that we were passed in, as our target
+	       parameter is only a hint.  */
+	    if (DECL_RTL (slot) != 0)
+	      {
+		target = DECL_RTL (slot);
+		/* If we have already expanded the slot, so don't do
+                   it again.  (mrs)  */
+		if (TREE_OPERAND (exp, 1) == NULL_TREE)
+		  return target;
+	      }
+	    /* If target is unchanging, but slot is not, we could end up
+	       initializing the unchanging target through non-unchanging
+	       references.  */
+	    else if (RTX_UNCHANGING_P (target) && ! TREE_READONLY (slot))
+	      target = 0;
+	    else
+	      {
+		DECL_RTL (slot) = target;
+
+		/* If we must have an addressable slot, then make sure that
+		   the RTL that we just stored in slot is OK.  */
+		if (TREE_ADDRESSABLE (slot))
+		  put_var_into_stack (slot);
+	      }
+	  }
+
 	if (target == 0)
 	  {
 	    if (DECL_RTL (slot) != 0)
@@ -8306,33 +8340,6 @@ expand_expr (exp, target, tmode, modifie
 		if (TREE_OPERAND (exp, 2) == 0)
 		  TREE_OPERAND (exp, 2) = maybe_build_cleanup (slot);
 		cleanups = TREE_OPERAND (exp, 2);
-	      }
-	  }
-	else
-	  {
-	    /* This case does occur, when expanding a parameter which
-	       needs to be constructed on the stack.  The target
-	       is the actual stack address that we want to initialize.
-	       The function we call will perform the cleanup in this case.  */
-
-	    /* If we have already assigned it space, use that space,
-	       not target that we were passed in, as our target
-	       parameter is only a hint.  */
-	    if (DECL_RTL (slot) != 0)
-	      {
-		target = DECL_RTL (slot);
-		/* If we have already expanded the slot, so don't do
-                   it again.  (mrs)  */
-		if (TREE_OPERAND (exp, 1) == NULL_TREE)
-		  return target;
-	      }
-	    else
-	      {
-		DECL_RTL (slot) = target;
-		/* If we must have an addressable slot, then make sure that
-		   the RTL that we just stored in slot is OK.  */
-		if (TREE_ADDRESSABLE (slot))
-		  put_var_into_stack (slot);
 	      }
 	  }
 
--- expr.c.jj	Tue Mar 13 14:04:14 2001
+++ expr.c	Wed Mar 14 19:30:59 2001
@@ -8277,6 +8277,51 @@ expand_expr (exp, target, tmode, modifie
 	   knows that it should fix up those uses.  */
 	TREE_USED (slot) = 1;
 
+	if (target)
+	  {
+	    /* This case does occur, when expanding a parameter which
+	       needs to be constructed on the stack.  The target
+	       is the actual stack address that we want to initialize.
+	       The function we call will perform the cleanup in this case.  */
+
+	    /* If we have already assigned it space, use that space,
+	       not target that we were passed in, as our target
+	       parameter is only a hint.  */
+	    if (DECL_RTL (slot) != 0)
+	      {
+		target = DECL_RTL (slot);
+		/* If we have already expanded the slot, so don't do
+                   it again.  (mrs)  */
+		if (TREE_OPERAND (exp, 1) == NULL_TREE)
+		  return target;
+	      }
+	    else
+	      {
+		/* If target is unchanging, but slot is not, we could end up
+		   initializing the unchanging target through non-unchanging
+		   references.  */
+	        if (RTX_UNCHANGING_P (target) && ! TREE_READONLY (slot))
+		  {
+		    if (GET_CODE (target) == REG
+			|| (GET_CODE (target) == MEM
+			    && GET_CODE (XEXP (target, 0)) == ADDRESSOF
+			    && GET_CODE (XEXP (XEXP (target, 0), 0)) == REG))
+		      RTX_UNCHANGING_P (target) = 0;
+		    else
+		      target = 0;
+		  }
+		if (target)
+		  {
+		    DECL_RTL (slot) = target;
+
+		    /* If we must have an addressable slot, then make sure
+		       that the RTL that we just stored in slot is OK.  */
+		    if (TREE_ADDRESSABLE (slot))
+		      put_var_into_stack (slot);
+		  }
+	      }
+	  }
+
 	if (target == 0)
 	  {
 	    if (DECL_RTL (slot) != 0)
@@ -8306,33 +8351,6 @@ expand_expr (exp, target, tmode, modifie
 		if (TREE_OPERAND (exp, 2) == 0)
 		  TREE_OPERAND (exp, 2) = maybe_build_cleanup (slot);
 		cleanups = TREE_OPERAND (exp, 2);
-	      }
-	  }
-	else
-	  {
-	    /* This case does occur, when expanding a parameter which
-	       needs to be constructed on the stack.  The target
-	       is the actual stack address that we want to initialize.
-	       The function we call will perform the cleanup in this case.  */
-
-	    /* If we have already assigned it space, use that space,
-	       not target that we were passed in, as our target
-	       parameter is only a hint.  */
-	    if (DECL_RTL (slot) != 0)
-	      {
-		target = DECL_RTL (slot);
-		/* If we have already expanded the slot, so don't do
-                   it again.  (mrs)  */
-		if (TREE_OPERAND (exp, 1) == NULL_TREE)
-		  return target;
-	      }
-	    else
-	      {
-		DECL_RTL (slot) = target;
-		/* If we must have an addressable slot, then make sure that
-		   the RTL that we just stored in slot is OK.  */
-		if (TREE_ADDRESSABLE (slot))
-		  put_var_into_stack (slot);
 	      }
 	  }