Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable
2012-04-16 Martin Jambor mjam...@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
Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable
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 mjam...@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 mjam...@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
Hi, On Fri, Apr 06, 2012 at 06:13:20PM +0200, Eric Botcazou wrote: 2012-04-03 Martin Jambor mjam...@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 mjam...@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
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 mjam...@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
2012-04-03 Martin Jambor mjam...@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
@@ -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
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 mjam...@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
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
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
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
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
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));
