[PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Martin Jambor]

Hi,

when testing a patch of mine on sparc64-linux, I came across an Ada
bootstrap failure due to a structure DECL which was marked addressable
but had a register DECL_RTL (and therefore mem_ref_refers_to_non_mem_p
failed to trigger on it).

Mode of the structure was TI (16 bytes int) and it was mistakenly
marked as addressable during expansion of an assignment statement in
which a 12 byte portion of it was copied to another structure with
BLKmode.  Specifically, this happened because expand_assignment called
store_expr which loaded the required portion to temporary 12 byte
BLKmode MEM_P variable and then called emit_block_move from the
temporary to the destination.  emit_block_move_hints then marked
MEM_EXPR of the temp as addressable because it handled the copy by
emitting a library call.  And MEM_EXPR pointed to the DECL of the
source of the assignment which I believe is the bug, thus this patch
re-sets MEM_EXPR of temp in these cases.

However, if anybody believes the main issue is elsewhere and another
component of this chain of events needs to be fixed, I'll be happy to
come up with another patch.  so far this patch has passed bootstrap
and testing on x86_64-linux and helped my patch which uncovered this
issue to reach stage 3 of bootstrap.

What do you think, is it OK for trunk?

Thanks,

Martin


2012-03-30  Martin Jambor  <mjambor@suse.cz>

	* expr.c (non_mem_decl_p): New function with half of previous
	functionality of...
	(mem_ref_refers_to_non_mem_p): ...this one.
	(store_expr): Reset MEM_EXPR of temp if it refers to memory but the
	original expression is based on a non-memory based declaration.

Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -4479,6 +4479,24 @@ get_bit_range (unsigned HOST_WIDE_INT *b
   *bitend = *bitstart + tree_low_cst (DECL_SIZE (repr), 1) - 1;
 }
 
+/* Returns true if T is a DECL that does not reside in a memory and has a
+   non-BLK mode.  */
+
+static bool
+non_mem_decl_p (tree t)
+{
+  if (!DECL_P (t))
+    return false;
+  gcc_checking_assert (!TREE_ADDRESSABLE (t)
+		       || !DECL_RTL_SET_P (t)
+		       || MEM_P (DECL_RTL (t)));
+  return (!TREE_ADDRESSABLE (t)
+	  && DECL_MODE (t) != BLKmode
+	  && DECL_RTL_SET_P (t)
+	  && !MEM_P (DECL_RTL (t)));
+}
+
+
 /* Returns true if the MEM_REF REF refers to an object that does not
    reside in memory and has non-BLKmode.  */
 
@@ -4489,11 +4507,7 @@ mem_ref_refers_to_non_mem_p (tree ref)
   if (TREE_CODE (base) != ADDR_EXPR)
     return false;
   base = TREE_OPERAND (base, 0);
-  return (DECL_P (base)
-	  && !TREE_ADDRESSABLE (base)
-	  && DECL_MODE (base) != BLKmode
-	  && DECL_RTL_SET_P (base)
-	  && !MEM_P (DECL_RTL (base)));
+  return non_mem_decl_p (base);
 }
 
 /* Expand an assignment that stores the value of FROM into TO.  If NONTEMPORAL
@@ -5105,6 +5119,16 @@ store_expr (tree exp, rtx target, int ca
 			       &alt_rtl);
     }
 
+  /* When the original expression is a declaration not residing in memory but
+     temp is a MEM RTX, we must dissociate it from the original expression or
+     emit_block_move might mark it as addressable.  */
+  if (MEM_P (temp))
+    {
+      tree base = get_base_address (exp);
+      if (base && non_mem_decl_p (base))
+	set_mem_expr (temp, NULL_TREE);
+    }
+
   /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not
      the same as that of TARGET, adjust the constant.  This is needed, for
      example, in case it is a CONST_DOUBLE and we want only a word-sized

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4192 bytes --]

On Fri, 30 Mar 2012, Martin Jambor wrote:

> Hi,
> 
> when testing a patch of mine on sparc64-linux, I came across an Ada
> bootstrap failure due to a structure DECL which was marked addressable
> but had a register DECL_RTL (and therefore mem_ref_refers_to_non_mem_p
> failed to trigger on it).
> 
> Mode of the structure was TI (16 bytes int) and it was mistakenly
> marked as addressable during expansion of an assignment statement in
> which a 12 byte portion of it was copied to another structure with
> BLKmode.  Specifically, this happened because expand_assignment called
> store_expr which loaded the required portion to temporary 12 byte
> BLKmode MEM_P variable and then called emit_block_move from the
> temporary to the destination.  emit_block_move_hints then marked
> MEM_EXPR of the temp as addressable because it handled the copy by
> emitting a library call.  And MEM_EXPR pointed to the DECL of the
> source of the assignment which I believe is the bug, thus this patch
> re-sets MEM_EXPR of temp in these cases.
> 
> However, if anybody believes the main issue is elsewhere and another
> component of this chain of events needs to be fixed, I'll be happy to
> come up with another patch.  so far this patch has passed bootstrap
> and testing on x86_64-linux and helped my patch which uncovered this
> issue to reach stage 3 of bootstrap.
> 
> What do you think, is it OK for trunk?

Hmm, I think this should be done at the point we create the mem - where
does that happen?

Richard.

> Thanks,
> 
> Martin
> 
> 
> 2012-03-30  Martin Jambor  <mjambor@suse.cz>
> 
> 	* expr.c (non_mem_decl_p): New function with half of previous
> 	functionality of...
> 	(mem_ref_refers_to_non_mem_p): ...this one.
> 	(store_expr): Reset MEM_EXPR of temp if it refers to memory but the
> 	original expression is based on a non-memory based declaration.
> 
> Index: src/gcc/expr.c
> ===================================================================
> --- src.orig/gcc/expr.c
> +++ src/gcc/expr.c
> @@ -4479,6 +4479,24 @@ get_bit_range (unsigned HOST_WIDE_INT *b
>    *bitend = *bitstart + tree_low_cst (DECL_SIZE (repr), 1) - 1;
>  }
>  
> +/* Returns true if T is a DECL that does not reside in a memory and has a
> +   non-BLK mode.  */
> +
> +static bool
> +non_mem_decl_p (tree t)
> +{
> +  if (!DECL_P (t))
> +    return false;
> +  gcc_checking_assert (!TREE_ADDRESSABLE (t)
> +		       || !DECL_RTL_SET_P (t)
> +		       || MEM_P (DECL_RTL (t)));
> +  return (!TREE_ADDRESSABLE (t)
> +	  && DECL_MODE (t) != BLKmode
> +	  && DECL_RTL_SET_P (t)
> +	  && !MEM_P (DECL_RTL (t)));
> +}
> +
> +
>  /* Returns true if the MEM_REF REF refers to an object that does not
>     reside in memory and has non-BLKmode.  */
>  
> @@ -4489,11 +4507,7 @@ mem_ref_refers_to_non_mem_p (tree ref)
>    if (TREE_CODE (base) != ADDR_EXPR)
>      return false;
>    base = TREE_OPERAND (base, 0);
> -  return (DECL_P (base)
> -	  && !TREE_ADDRESSABLE (base)
> -	  && DECL_MODE (base) != BLKmode
> -	  && DECL_RTL_SET_P (base)
> -	  && !MEM_P (DECL_RTL (base)));
> +  return non_mem_decl_p (base);
>  }
>  
>  /* Expand an assignment that stores the value of FROM into TO.  If NONTEMPORAL
> @@ -5105,6 +5119,16 @@ store_expr (tree exp, rtx target, int ca
>  			       &alt_rtl);
>      }
>  
> +  /* When the original expression is a declaration not residing in memory but
> +     temp is a MEM RTX, we must dissociate it from the original expression or
> +     emit_block_move might mark it as addressable.  */
> +  if (MEM_P (temp))
> +    {
> +      tree base = get_base_address (exp);
> +      if (base && non_mem_decl_p (base))
> +	set_mem_expr (temp, NULL_TREE);
> +    }
> +
>    /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not
>       the same as that of TARGET, adjust the constant.  This is needed, for
>       example, in case it is a CONST_DOUBLE and we want only a word-sized
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Martin Jambor]

Hi,

On Fri, Mar 30, 2012 at 10:03:59AM +0200, Richard Guenther wrote:
> On Fri, 30 Mar 2012, Martin Jambor wrote:
> 
> > Hi,
> > 
> > when testing a patch of mine on sparc64-linux, I came across an Ada
> > bootstrap failure due to a structure DECL which was marked addressable
> > but had a register DECL_RTL (and therefore mem_ref_refers_to_non_mem_p
> > failed to trigger on it).
> > 
> > Mode of the structure was TI (16 bytes int) and it was mistakenly
> > marked as addressable during expansion of an assignment statement in
> > which a 12 byte portion of it was copied to another structure with
> > BLKmode.  Specifically, this happened because expand_assignment called
> > store_expr which loaded the required portion to temporary 12 byte
> > BLKmode MEM_P variable and then called emit_block_move from the
> > temporary to the destination.  emit_block_move_hints then marked
> > MEM_EXPR of the temp as addressable because it handled the copy by
> > emitting a library call.  And MEM_EXPR pointed to the DECL of the
> > source of the assignment which I believe is the bug, thus this patch
> > re-sets MEM_EXPR of temp in these cases.
> > 
> > However, if anybody believes the main issue is elsewhere and another
> > component of this chain of events needs to be fixed, I'll be happy to
> > come up with another patch.  so far this patch has passed bootstrap
> > and testing on x86_64-linux and helped my patch which uncovered this
> > issue to reach stage 3 of bootstrap.
> > 
> > What do you think, is it OK for trunk?
> 
> Hmm, I think this should be done at the point we create the mem - where
> does that happen?

The function gets it by simply calling expand_expr_real:

      temp = expand_expr_real (exp, tmp_target, GET_MODE (target),
			       (call_param_p
				? EXPAND_STACK_PARM : EXPAND_NORMAL),
			       &alt_rtl);

I have reproduced the issue again and looked at how expand_expr_real_1
comes up with the MEM attributes in the COMPONENT_REF case.
The memory-backed temporary is created in:

	/* Otherwise, if this is a constant or the object is not in memory
	   and need be, put it there.  */
	else if (CONSTANT_P (op0) || (!MEM_P (op0) && must_force_mem))
	  {
	    tree nt = build_qualified_type (TREE_TYPE (tem),
					    (TYPE_QUALS (TREE_TYPE (tem))
					     | TYPE_QUAL_CONST));
	    memloc = assign_temp (nt, 1, 1, 1);
	    emit_move_insn (memloc, op0);
	    op0 = memloc;
	  }

and at the end of the  case, the bitpos is added by adjust_address,
followed by set_mem_attributes (op0, exp, 0);

Indeed it seems that we should not be calling set_mem_attributes if
op0 is based on such temporary... or perhaps just make sure that we
only clear MEM_EXPR afterwards?

For example, the following patch also passes bootstrap and testing on
x86_64-linux, bootstrap on sparc64 is still running (and IMHO has not
yet reached the critical point).

Thanks,

Martin



Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 185792)
+++ gcc/expr.c	(working copy)
@@ -9564,6 +9564,7 @@ expand_expr_real_1 (tree exp, rtx target
 	tree tem = get_inner_reference (exp, &bitsize, &bitpos, &offset,
 					&mode1, &unsignedp, &volatilep, true);
 	rtx orig_op0, memloc;
+	bool do_set_mem_attrs = true;
 
 	/* If we got back the original object, something is wrong.  Perhaps
 	   we are evaluating an expression too early.  In any event, don't
@@ -9669,6 +9670,7 @@ expand_expr_real_1 (tree exp, rtx target
 	    memloc = assign_temp (nt, 1, 1, 1);
 	    emit_move_insn (memloc, op0);
 	    op0 = memloc;
+	    do_set_mem_attrs = false;
 	  }
 
 	if (offset)
@@ -9841,7 +9843,8 @@ expand_expr_real_1 (tree exp, rtx target
 		emit_move_insn (new_rtx, op0);
 		op0 = copy_rtx (new_rtx);
 		PUT_MODE (op0, BLKmode);
-		set_mem_attributes (op0, exp, 1);
+		if (do_set_mem_attrs)
+		  set_mem_attributes (op0, exp, 1);
 	      }
 
 	    return op0;
@@ -9862,7 +9865,8 @@ expand_expr_real_1 (tree exp, rtx target
 	if (op0 == orig_op0)
 	  op0 = copy_rtx (op0);
 
-	set_mem_attributes (op0, exp, 0);
+	if (do_set_mem_attrs)
+	  set_mem_attributes (op0, exp, 0);
 	if (REG_P (XEXP (op0, 0)))
 	  mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0));
 

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Richard Guenther]

On Sat, 31 Mar 2012, Martin Jambor wrote:

> Hi,
> 
> On Fri, Mar 30, 2012 at 10:03:59AM +0200, Richard Guenther wrote:
> > On Fri, 30 Mar 2012, Martin Jambor wrote:
> > 
> > > Hi,
> > > 
> > > when testing a patch of mine on sparc64-linux, I came across an Ada
> > > bootstrap failure due to a structure DECL which was marked addressable
> > > but had a register DECL_RTL (and therefore mem_ref_refers_to_non_mem_p
> > > failed to trigger on it).
> > > 
> > > Mode of the structure was TI (16 bytes int) and it was mistakenly
> > > marked as addressable during expansion of an assignment statement in
> > > which a 12 byte portion of it was copied to another structure with
> > > BLKmode.  Specifically, this happened because expand_assignment called
> > > store_expr which loaded the required portion to temporary 12 byte
> > > BLKmode MEM_P variable and then called emit_block_move from the
> > > temporary to the destination.  emit_block_move_hints then marked
> > > MEM_EXPR of the temp as addressable because it handled the copy by
> > > emitting a library call.  And MEM_EXPR pointed to the DECL of the
> > > source of the assignment which I believe is the bug, thus this patch
> > > re-sets MEM_EXPR of temp in these cases.
> > > 
> > > However, if anybody believes the main issue is elsewhere and another
> > > component of this chain of events needs to be fixed, I'll be happy to
> > > come up with another patch.  so far this patch has passed bootstrap
> > > and testing on x86_64-linux and helped my patch which uncovered this
> > > issue to reach stage 3 of bootstrap.
> > > 
> > > What do you think, is it OK for trunk?
> > 
> > Hmm, I think this should be done at the point we create the mem - where
> > does that happen?
> 
> The function gets it by simply calling expand_expr_real:
> 
>       temp = expand_expr_real (exp, tmp_target, GET_MODE (target),
> 			       (call_param_p
> 				? EXPAND_STACK_PARM : EXPAND_NORMAL),
> 			       &alt_rtl);
> 
> I have reproduced the issue again and looked at how expand_expr_real_1
> comes up with the MEM attributes in the COMPONENT_REF case.
> The memory-backed temporary is created in:
> 
> 	/* Otherwise, if this is a constant or the object is not in memory
> 	   and need be, put it there.  */
> 	else if (CONSTANT_P (op0) || (!MEM_P (op0) && must_force_mem))
> 	  {
> 	    tree nt = build_qualified_type (TREE_TYPE (tem),
> 					    (TYPE_QUALS (TREE_TYPE (tem))
> 					     | TYPE_QUAL_CONST));
> 	    memloc = assign_temp (nt, 1, 1, 1);
> 	    emit_move_insn (memloc, op0);
> 	    op0 = memloc;
> 	  }
> 
> and at the end of the  case, the bitpos is added by adjust_address,
> followed by set_mem_attributes (op0, exp, 0);
> 
> Indeed it seems that we should not be calling set_mem_attributes if
> op0 is based on such temporary... or perhaps just make sure that we
> only clear MEM_EXPR afterwards?

Yes, either way I suppose.  The following also looks dangerous to me:

        /* If OFFSET is making OP0 more aligned than BIGGEST_ALIGNMENT,
           record its alignment as BIGGEST_ALIGNMENT.  */
        if (MEM_P (op0) && bitpos == 0 && offset != 0
            && is_aligning_offset (offset, tem))
          set_mem_align (op0, BIGGEST_ALIGNMENT);

Maybe we can fall through most of the rest of the function if we
canonicalized in the above way?  Eric?

Thanks,
Richard.

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Eric Botcazou]

> Yes, either way I suppose.  The following also looks dangerous to me:
>
>         /* If OFFSET is making OP0 more aligned than BIGGEST_ALIGNMENT,
>            record its alignment as BIGGEST_ALIGNMENT.  */
>         if (MEM_P (op0) && bitpos == 0 && offset != 0
>             && is_aligning_offset (offset, tem))
>           set_mem_align (op0, BIGGEST_ALIGNMENT);
>
> Maybe we can fall through most of the rest of the function if we
> canonicalized in the above way?  Eric?

Probably not, I'm afraid.  I agree that the above call to set_mem_align is 
potentially problematic if we previously allocated the temp.  Moreover, I 
think that the other temp allocation around line 9840 is problematic too.

On the other hand, we could avoid skipping set_mem_attributes entirely by 
passing the type instead of the expression.

So I'd set a flag for the first temp allocation, skip the set_mem_align call if 
it is set and pass the type instead of the expression in the final call to 
set_mem_attributes if it is set.  And I'd handle the second temp allocation 
independently and pass the type here too.

-- 
Eric Botcazou

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Richard Guenther]

On Tue, 3 Apr 2012, Eric Botcazou wrote:

> > Yes, either way I suppose.  The following also looks dangerous to me:
> >
> >         /* If OFFSET is making OP0 more aligned than BIGGEST_ALIGNMENT,
> >            record its alignment as BIGGEST_ALIGNMENT.  */
> >         if (MEM_P (op0) && bitpos == 0 && offset != 0
> >             && is_aligning_offset (offset, tem))
> >           set_mem_align (op0, BIGGEST_ALIGNMENT);
> >
> > Maybe we can fall through most of the rest of the function if we
> > canonicalized in the above way?  Eric?
> 
> Probably not, I'm afraid.  I agree that the above call to set_mem_align is 
> potentially problematic if we previously allocated the temp.  Moreover, I 
> think that the other temp allocation around line 9840 is problematic too.
> 
> On the other hand, we could avoid skipping set_mem_attributes entirely by 
> passing the type instead of the expression.
> 
> So I'd set a flag for the first temp allocation, skip the set_mem_align call if 
> it is set and pass the type instead of the expression in the final call to 
> set_mem_attributes if it is set.  And I'd handle the second temp allocation 
> independently and pass the type here too.

Yeah, that sounds reasonable.

Richard.

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Eric Botcazou]

> Yeah, that sounds reasonable.

There is a further subtlety in the second temp allocation when the expression 
doesn't use the alias set of its type.  In that case, we cannot pass the type 
to set_mem_attributes.  In fact, since assign_stack_temp_for_type already 
calls the appropriate set_mem_* routines, the best thing to do might be to 
remove the call to set_mem_attributes altogether in that case.

-- 
Eric Botcazou

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Martin Jambor]

Hi,

On Tue, Apr 03, 2012 at 11:02:11AM +0200, Eric Botcazou wrote:
> > Yeah, that sounds reasonable.
> 
> There is a further subtlety in the second temp allocation when the expression 
> doesn't use the alias set of its type.  In that case, we cannot pass the type 
> to set_mem_attributes.  In fact, since assign_stack_temp_for_type already 
> calls the appropriate set_mem_* routines, the best thing to do might be to 
> remove the call to set_mem_attributes altogether in that case.
> 

So, something like this?  Bootstrapped and tested on x86_64-linux and
ia64-linux, I'm currently having problems bootsrapping sparc64 which
is what I need this mainly for but those are unelated and this should
help.

Thanks,

Martin



2012-04-03  Martin Jambor  <mjambor@suse.cz>

	* expr.c (expand_expr_real_1): Pass type, not the expression, to
	set_mem_attributes for a memory temporary.  Do not call the
	function for temporaries with a different alias set.

Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -9572,6 +9572,7 @@ expand_expr_real_1 (tree exp, rtx target
 	tree tem = get_inner_reference (exp, &bitsize, &bitpos, &offset,
 					&mode1, &unsignedp, &volatilep, true);
 	rtx orig_op0, memloc;
+	bool mem_attrs_from_type = false;
 
 	/* If we got back the original object, something is wrong.  Perhaps
 	   we are evaluating an expression too early.  In any event, don't
@@ -9677,6 +9678,7 @@ expand_expr_real_1 (tree exp, rtx target
 	    memloc = assign_temp (nt, 1, 1, 1);
 	    emit_move_insn (memloc, op0);
 	    op0 = memloc;
+	    mem_attrs_from_type = true;
 	  }
 
 	if (offset)
@@ -9849,7 +9851,6 @@ expand_expr_real_1 (tree exp, rtx target
 		emit_move_insn (new_rtx, op0);
 		op0 = copy_rtx (new_rtx);
 		PUT_MODE (op0, BLKmode);
-		set_mem_attributes (op0, exp, 1);
 	      }
 
 	    return op0;
@@ -9870,7 +9871,14 @@ expand_expr_real_1 (tree exp, rtx target
 	if (op0 == orig_op0)
 	  op0 = copy_rtx (op0);
 
-	set_mem_attributes (op0, exp, 0);
+	/* If op0 is a temporary because of forcing to memory, pass only the
+	   type to set_mem_attributes so that the original expression is never
+	   marked as ADDRESSABLE through MEM_EXPR of the temporary.  */
+	if (mem_attrs_from_type)
+	  set_mem_attributes (op0, TREE_TYPE (exp), 0);
+	else
+	  set_mem_attributes (op0, exp, 0);
+
 	if (REG_P (XEXP (op0, 0)))
 	  mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0));
 

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Eric Botcazou]

> 2012-04-03  Martin Jambor  <mjambor@suse.cz>
>
> 	* expr.c (expand_expr_real_1): Pass type, not the expression, to
> 	set_mem_attributes for a memory temporary.  Do not call the
> 	function for temporaries with a different alias set.

The last sentence is unprecise, this would rather be: "Do not call the function 
for the memory temporary created for a bitfield".

I wonder whether we should simplify the bitfield case in the process.  Once we 
remove the call to set_mem_attributes, I think the

		/* If the reference doesn't use the alias set of its type,
		   we cannot create the temporary using that type.  */

is useless, so we could try to revert r122014 in the process.

-- 
Eric Botcazou

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Eric Botcazou]

> @@ -9870,7 +9871,14 @@ expand_expr_real_1 (tree exp, rtx target
>  	if (op0 == orig_op0)
>  	  op0 = copy_rtx (op0);
>
> -	set_mem_attributes (op0, exp, 0);
> +	/* If op0 is a temporary because of forcing to memory, pass only the
> +	   type to set_mem_attributes so that the original expression is never
> +	   marked as ADDRESSABLE through MEM_EXPR of the temporary.  */
> +	if (mem_attrs_from_type)
> +	  set_mem_attributes (op0, TREE_TYPE (exp), 0);

set_mem_attributes (op0, type, 0); is equivalent.

-- 
Eric Botcazou

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Martin Jambor]

Hi,

On Fri, Apr 06, 2012 at 06:13:20PM +0200, Eric Botcazou wrote:
> > 2012-04-03  Martin Jambor  <mjambor@suse.cz>
> >
> > 	* expr.c (expand_expr_real_1): Pass type, not the expression, to
> > 	set_mem_attributes for a memory temporary.  Do not call the
> > 	function for temporaries with a different alias set.
> 
> The last sentence is unprecise, this would rather be: "Do not call
> the function for the memory temporary created for a bitfield".

OK

> 
> I wonder whether we should simplify the bitfield case in the process.  Once we 
> remove the call to set_mem_attributes, I think the
> 
> 		/* If the reference doesn't use the alias set of its type,
> 		   we cannot create the temporary using that type.  */
> 
> is useless, so we could try to revert r122014 in the process.

Well, the commit did not add a testcase and when I looked up the patch
in the mailing list archive
(http://gcc.gnu.org/ml/gcc-patches/2006-11/msg01449.html) it said it
was fixing problems not reproducible on trunk so it's basically
impossible for me to evaluate whether it is still necessary by some
simple testing.  Having said that, I guess I can give it a round of
regular testing on all the platforms I have currently set up.

On Fri, Apr 06, 2012 at 06:21:55PM +0200, Eric Botcazou wrote:
> > @@ -9870,7 +9871,14 @@ expand_expr_real_1 (tree exp, rtx target
> >  	if (op0 == orig_op0)
> >  	  op0 = copy_rtx (op0);
> >
> > -	set_mem_attributes (op0, exp, 0);
> > +	/* If op0 is a temporary because of forcing to memory, pass only the
> > +	   type to set_mem_attributes so that the original expression is never
> > +	   marked as ADDRESSABLE through MEM_EXPR of the temporary.  */
> > +	if (mem_attrs_from_type)
> > +	  set_mem_attributes (op0, TREE_TYPE (exp), 0);
> 
> set_mem_attributes (op0, type, 0); is equivalent.
> 

OK.

So basically I'd like to commit the following.  It has been
successfully bootstrapped and tested on x86_64-linux, i686-linux,
sparc64-linux (without Java), ia64-linux (without Ada) and tested on
hppa-linux (C and C++ only).

OK for trunk?

Thanks,

Martin


2012-04-10  Martin Jambor  <mjambor@suse.cz>

	* expr.c (expand_expr_real_1): Pass type, not the expression, to
	set_mem_attributes for a memory temporary. Do not call the function
	for the memory temporary created for a bitfield.

Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -9598,6 +9598,7 @@ expand_expr_real_1 (tree exp, rtx target
 	tree tem = get_inner_reference (exp, &bitsize, &bitpos, &offset,
 					&mode1, &unsignedp, &volatilep, true);
 	rtx orig_op0, memloc;
+	bool mem_attrs_from_type = false;
 
 	/* If we got back the original object, something is wrong.  Perhaps
 	   we are evaluating an expression too early.  In any event, don't
@@ -9703,6 +9704,7 @@ expand_expr_real_1 (tree exp, rtx target
 	    memloc = assign_temp (nt, 1, 1, 1);
 	    emit_move_insn (memloc, op0);
 	    op0 = memloc;
+	    mem_attrs_from_type = true;
 	  }
 
 	if (offset)
@@ -9875,7 +9877,6 @@ expand_expr_real_1 (tree exp, rtx target
 		emit_move_insn (new_rtx, op0);
 		op0 = copy_rtx (new_rtx);
 		PUT_MODE (op0, BLKmode);
-		set_mem_attributes (op0, exp, 1);
 	      }
 
 	    return op0;
@@ -9896,7 +9897,14 @@ expand_expr_real_1 (tree exp, rtx target
 	if (op0 == orig_op0)
 	  op0 = copy_rtx (op0);
 
-	set_mem_attributes (op0, exp, 0);
+	/* If op0 is a temporary because of forcing to memory, pass only the
+	   type to set_mem_attributes so that the original expression is never
+	   marked as ADDRESSABLE through MEM_EXPR of the temporary.  */
+	if (mem_attrs_from_type)
+	  set_mem_attributes (op0, type, 0);
+	else
+	  set_mem_attributes (op0, exp, 0);
+
 	if (REG_P (XEXP (op0, 0)))
 	  mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0));
 

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Eric Botcazou]

> Well, the commit did not add a testcase and when I looked up the patch
> in the mailing list archive
> (http://gcc.gnu.org/ml/gcc-patches/2006-11/msg01449.html) it said it
> was fixing problems not reproducible on trunk so it's basically
> impossible for me to evaluate whether it is still necessary by some
> simple testing.  Having said that, I guess I can give it a round of
> regular testing on all the platforms I have currently set up.

The problem was that, for the same address, you had the alias set of the type 
on one MEM and the alias set of the reference on the other MEM.  If the alias 
set of the reference doesn't conflict with that of the type (this can happen 
in Ada because of DECL_NONADDRESSABLE_P), the RAW dependency may be missed.

If we don't put the alias set of the reference on one of the MEM, then I don't 
think that we need to put it on the other MEM.  That's what's done for the 
first, non-bitfield temporary now.

> 2012-04-10  Martin Jambor  <mjambor@suse.cz>
>
> 	* expr.c (expand_expr_real_1): Pass type, not the expression, to
> 	set_mem_attributes for a memory temporary. Do not call the function
> 	for the memory temporary created for a bitfield.

Fine with me, but the now dangling code in the bitfield case is a bit annoying.

-- 
Eric Botcazou

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Martin Jambor]

Hi,

On Thu, Apr 12, 2012 at 07:21:12PM +0200, Eric Botcazou wrote:
> > Well, the commit did not add a testcase and when I looked up the patch
> > in the mailing list archive
> > (http://gcc.gnu.org/ml/gcc-patches/2006-11/msg01449.html) it said it
> > was fixing problems not reproducible on trunk so it's basically
> > impossible for me to evaluate whether it is still necessary by some
> > simple testing.  Having said that, I guess I can give it a round of
> > regular testing on all the platforms I have currently set up.
> 
> The problem was that, for the same address, you had the alias set of the type 
> on one MEM and the alias set of the reference on the other MEM.  If the alias 
> set of the reference doesn't conflict with that of the type (this can happen 
> in Ada because of DECL_NONADDRESSABLE_P), the RAW dependency may be missed.
> 
> If we don't put the alias set of the reference on one of the MEM, then I don't 
> think that we need to put it on the other MEM.  That's what's done for the 
> first, non-bitfield temporary now.
> 
> > 2012-04-10  Martin Jambor  <mjambor@suse.cz>
> >
> > 	* expr.c (expand_expr_real_1): Pass type, not the expression, to
> > 	set_mem_attributes for a memory temporary. Do not call the function
> > 	for the memory temporary created for a bitfield.
> 
> Fine with me, but the now dangling code in the bitfield case is a bit annoying.

In order to alleviate that feeling, I'd like to propose the following
patch, which I have successfully bootstrapped and tested on
x86_64-linux (including Ada and obj-c++), i686-linux (likewise),
sparc64-linux (with Ada but without Java), ia64-linux (default
languages, i.e. without Ada) and ppc64-linux (likewise).  Testsuite
run (no bootstrap) on hppa-linux (C and C++ only) is still running and
I expect to have results tomorrow.

Thus, OK for trunk?

Thanks,

Martin


2012-04-16  Martin Jambor  <mjambor@suse.cz>

	* expr.c (expand_expr_real_1): Remove setting parent's alias set for
	temporaries created for a bitfield (reverting revision 122014).

Index: src/gcc/expr.c
===================================================================
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -9866,19 +9866,11 @@ expand_expr_real_1 (tree exp, rtx target
 	       necessarily be constant.  */
 	    if (mode == BLKmode)
 	      {
-		HOST_WIDE_INT size = GET_MODE_BITSIZE (ext_mode);
 		rtx new_rtx;
 
-		/* If the reference doesn't use the alias set of its type,
-		   we cannot create the temporary using that type.  */
-		if (component_uses_parent_alias_set (exp))
-		  {
-		    new_rtx = assign_stack_local (ext_mode, size, 0);
-		    set_mem_alias_set (new_rtx, get_alias_set (exp));
-		  }
-		else
-		  new_rtx = assign_stack_temp_for_type (ext_mode, size, 0, type);
-
+		new_rtx = assign_stack_temp_for_type (ext_mode,
+						   GET_MODE_BITSIZE (ext_mode),
+						   0, type);
 		emit_move_insn (new_rtx, op0);
 		op0 = copy_rtx (new_rtx);
 		PUT_MODE (op0, BLKmode);

Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Eric Botcazou]

> 2012-04-16  Martin Jambor  <mjambor@suse.cz>
> 
> 	* expr.c (expand_expr_real_1): Remove setting parent's alias set for
> 	temporaries created for a bitfield (reverting revision 122014).

OK, thanks.

-- 
Eric Botcazou