* RFC/A: set_mem_attributes_minus_bitpos tweak
@ 2012-11-03 11:53 Richard Sandiford
2012-11-07 9:54 ` Eric Botcazou
0 siblings, 1 reply; 4+ messages in thread
From: Richard Sandiford @ 2012-11-03 11:53 UTC (permalink / raw)
To: gcc-patches
While working on the insertion and extraction optabs, I came across
some strange-looking behaviour relatd to set_mem_attributes_minus_bitpos.
In gcc.c-torture/execute/20030714-1.c we have:
struct RenderStyle
{
struct NonInheritedFlags
{
union
{
struct
{
unsigned int _display : 4;
unsigned int _bg_repeat : 2;
bool _bg_attachment : 1;
unsigned int _overflow : 4 ;
unsigned int _vertical_align : 4;
unsigned int _clear : 2;
EPosition _position : 2;
EFloat _floating : 2;
unsigned int _table_layout : 1;
bool _flowAroundFloats :1;
unsigned int _styleType : 3;
bool _hasHover : 1;
bool _hasActive : 1;
bool _clipSpecified : 1;
unsigned int _unicodeBidi : 2;
int _unused : 1;
} f;
int _niflags;
};
} noninherited_flags;
};
...
RenderStyle g__style;
...
g__style.noninherited_flags.f._position = FIXED;
...
expand_assignment calls:
if (MEM_P (to_rtx))
{
/* If the field is at offset zero, we could have been given the
DECL_RTX of the parent struct. Don't munge it. */
to_rtx = shallow_copy_rtx (to_rtx);
set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
...
}
where "to_rtx" is:
(mem/c:SI (symbol_ref:DI ("g__style") [flags 0x4] <var_decl 0x7ffff6e4ee40 g__style>) [0 g__style+0 S4 A64])
and "to" is the component_ref. So far so good.
But set_mem_attributes_minus_bitpos has:
/* Default values from pre-existing memory attributes if present. */
refattrs = MEM_ATTRS (ref);
if (refattrs)
{
/* ??? Can this ever happen? Calling this routine on a MEM that
already carries memory attributes should probably be invalid. */
attrs.expr = refattrs->expr;
attrs.offset_known_p = refattrs->offset_known_p;
attrs.offset = refattrs->offset;
attrs.size_known_p = refattrs->size_known_p;
attrs.size = refattrs->size;
attrs.align = refattrs->align;
}
which of course applies in this case: we have a MEM for g__style.
I would expect many calls from this site are for MEMs with an
existing MEM_EXPR.
So the new attributes also start out describing g__style. Then we have:
/* If the size is known, we can set that. */
if (TYPE_SIZE_UNIT (type) && host_integerp (TYPE_SIZE_UNIT (type), 1))
{
attrs.size_known_p = true;
attrs.size = tree_low_cst (TYPE_SIZE_UNIT (type), 1);
}
which sets the size to 1 byte (because that's all the bitfield occupies).
Then later:
/* If this is a field reference and not a bit-field, record it. */
/* ??? There is some information that can be gleaned from bit-fields,
such as the word offset in the structure that might be modified.
But skip it for now. */
else if (TREE_CODE (t) == COMPONENT_REF
&& ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
so we leave the offset and expr alone. The end result is:
(mem/j/c:SI (symbol_ref:DI ("g__style") [flags 0x4] <var_decl 0x7ffff6e4ee40 g__style>) [0 g__style+0 S1 A64])
an SImode reference to the first byte (and only the first byte) of g__style.
Then, when we apply adjust_bitfield_address, it looks like we're moving
past the end of the region and so we drop the MEM_EXPR.
In cases where set_mem_attributes_minus_bitpos does set MEM_EXPR based
on the new tree, it also adds the bitpos to the size. But I think we
should do that whenever we set the size based on the new tree,
regardless of whether we were able to record a MEM_EXPR too.
That said, this code has lots of ???s in it, so I'm not exactly
confident about this change. Thoughts?
Richard
gcc/
* emit-rtl.c (set_mem_attributes_minus_bitpos): If setting the
size based on the new tree, always include the bitpos in it.
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2012-11-01 20:43:37.193362850 +0000
+++ gcc/emit-rtl.c 2012-11-01 20:44:22.248362740 +0000
@@ -1569,7 +1569,7 @@ get_mem_align_offset (rtx mem, unsigned
set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp,
HOST_WIDE_INT bitpos)
{
- HOST_WIDE_INT apply_bitpos = 0;
+ tree expr = NULL_TREE;
tree type;
struct mem_attrs attrs, *defattrs, *refattrs;
addr_space_t as;
@@ -1679,11 +1679,7 @@ set_mem_attributes_minus_bitpos (rtx ref
attrs.align = MAX (attrs.align, TYPE_ALIGN (type));
/* If the size is known, we can set that. */
- if (TYPE_SIZE_UNIT (type) && host_integerp (TYPE_SIZE_UNIT (type), 1))
- {
- attrs.size_known_p = true;
- attrs.size = tree_low_cst (TYPE_SIZE_UNIT (type), 1);
- }
+ tree new_size = TYPE_SIZE_UNIT (type);
/* If T is not a type, we may be able to deduce some more information about
the expression. */
@@ -1738,17 +1734,10 @@ set_mem_attributes_minus_bitpos (rtx ref
/* If this is a decl, set the attributes of the MEM from it. */
if (DECL_P (t))
{
- attrs.expr = t;
+ expr = t;
attrs.offset_known_p = true;
attrs.offset = 0;
- apply_bitpos = bitpos;
- if (DECL_SIZE_UNIT (t) && host_integerp (DECL_SIZE_UNIT (t), 1))
- {
- attrs.size_known_p = true;
- attrs.size = tree_low_cst (DECL_SIZE_UNIT (t), 1);
- }
- else
- attrs.size_known_p = false;
+ new_size = DECL_SIZE_UNIT (t);
attrs.align = DECL_ALIGN (t);
align_computed = true;
}
@@ -1770,10 +1759,9 @@ set_mem_attributes_minus_bitpos (rtx ref
else if (TREE_CODE (t) == COMPONENT_REF
&& ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
{
- attrs.expr = t;
+ expr = t;
attrs.offset_known_p = true;
attrs.offset = 0;
- apply_bitpos = bitpos;
/* ??? Any reason the field size would be different than
the size we got from the type? */
}
@@ -1812,7 +1800,7 @@ set_mem_attributes_minus_bitpos (rtx ref
if (DECL_P (t2))
{
- attrs.expr = t2;
+ expr = t2;
attrs.offset_known_p = false;
if (host_integerp (off_tree, 1))
{
@@ -1824,18 +1812,16 @@ set_mem_attributes_minus_bitpos (rtx ref
align_computed = true;
attrs.offset_known_p = true;
attrs.offset = ioff;
- apply_bitpos = bitpos;
}
}
else if (TREE_CODE (t2) == COMPONENT_REF)
{
- attrs.expr = t2;
+ expr = t2;
attrs.offset_known_p = false;
if (host_integerp (off_tree, 1))
{
attrs.offset_known_p = true;
attrs.offset = tree_low_cst (off_tree, 1);
- apply_bitpos = bitpos;
}
/* ??? Any reason the field size would be different than
the size we got from the type? */
@@ -1846,10 +1832,9 @@ set_mem_attributes_minus_bitpos (rtx ref
else if (TREE_CODE (t) == MEM_REF
|| TREE_CODE (t) == TARGET_MEM_REF)
{
- attrs.expr = t;
+ expr = t;
attrs.offset_known_p = true;
attrs.offset = 0;
- apply_bitpos = bitpos;
}
if (!align_computed)
@@ -1861,17 +1846,22 @@ set_mem_attributes_minus_bitpos (rtx ref
else
as = TYPE_ADDR_SPACE (type);
- /* If we modified OFFSET based on T, then subtract the outstanding
- bit position offset. Similarly, increase the size of the accessed
- object to contain the negative offset. */
- if (apply_bitpos)
+ if (expr)
{
- gcc_assert (attrs.offset_known_p);
- attrs.offset -= apply_bitpos / BITS_PER_UNIT;
- if (attrs.size_known_p)
- attrs.size += apply_bitpos / BITS_PER_UNIT;
+ attrs.expr = expr;
+ /* If we modified OFFSET based on T, then subtract the outstanding
+ bit position offset. */
+ if (attrs.offset_known_p)
+ attrs.offset -= bitpos / BITS_PER_UNIT;
}
-
+ if (host_integerp (new_size, 1))
+ {
+ attrs.size_known_p = true;
+ /* Include the outstanding bit offset in the size, so that the
+ initial reference includes the leading gap and T. */
+ attrs.size = tree_low_cst (new_size, 1) + bitpos / BITS_PER_UNIT;
+ }
+
/* Now set the attributes we computed above. */
attrs.addrspace = as;
set_mem_attrs (ref, &attrs);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: RFC/A: set_mem_attributes_minus_bitpos tweak
2012-11-03 11:53 RFC/A: set_mem_attributes_minus_bitpos tweak Richard Sandiford
@ 2012-11-07 9:54 ` Eric Botcazou
2012-11-15 19:50 ` Richard Sandiford
0 siblings, 1 reply; 4+ messages in thread
From: Eric Botcazou @ 2012-11-07 9:54 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> expand_assignment calls:
>
> if (MEM_P (to_rtx))
> {
> /* If the field is at offset zero, we could have been given
> the DECL_RTX of the parent struct. Don't munge it. */ to_rtx =
> shallow_copy_rtx (to_rtx);
>
> set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
> ...
The MEM_KEEP_ALIAS_SET_P line seems to be redundant here (but not the
MEM_VOLATILE_P line).
> But set_mem_attributes_minus_bitpos has:
>
> /* Default values from pre-existing memory attributes if present. */
> refattrs = MEM_ATTRS (ref);
> if (refattrs)
> {
> /* ??? Can this ever happen? Calling this routine on a MEM that
> already carries memory attributes should probably be invalid. */
> attrs.expr = refattrs->expr;
> attrs.offset_known_p = refattrs->offset_known_p;
> attrs.offset = refattrs->offset;
> attrs.size_known_p = refattrs->size_known_p;
> attrs.size = refattrs->size;
> attrs.align = refattrs->align;
> }
>
> which of course applies in this case: we have a MEM for g__style.
> I would expect many calls from this site are for MEMs with an
> existing MEM_EXPR.
Indeed. Not very clear what to do though.
> Then later:
>
> /* If this is a field reference and not a bit-field, record it. */
> /* ??? There is some information that can be gleaned from bit-fields,
> such as the word offset in the structure that might be modified.
> But skip it for now. */
> else if (TREE_CODE (t) == COMPONENT_REF
> && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
>
> so we leave the offset and expr alone. The end result is:
>
> (mem/j/c:SI (symbol_ref:DI ("g__style") [flags 0x4] <var_decl
> 0x7ffff6e4ee40 g__style>) [0 g__style+0 S1 A64])
>
> an SImode reference to the first byte (and only the first byte) of g__style.
> Then, when we apply adjust_bitfield_address, it looks like we're moving
> past the end of the region and so we drop the MEM_EXPR.
>
> In cases where set_mem_attributes_minus_bitpos does set MEM_EXPR based
> on the new tree, it also adds the bitpos to the size. But I think we
> should do that whenever we set the size based on the new tree,
> regardless of whether we were able to record a MEM_EXPR too.
>
> That said, this code has lots of ???s in it, so I'm not exactly
> confident about this change. Thoughts?
It also seems a little bold to me. Since we now have the new processing of
MEM_EXPR for bitfields, can't we remove the ! DECL_BIT_FIELD check?
/* If this is a field reference and not a bit-field, record it. */
/* ??? There is some information that can be gleaned from bit-fields,
such as the word offset in the structure that might be modified.
But skip it for now. */
else if (TREE_CODE (t) == COMPONENT_REF
&& ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
{
attrs.expr = t;
attrs.offset_known_p = true;
attrs.offset = 0;
apply_bitpos = bitpos;
/* ??? Any reason the field size would be different than
the size we got from the type? */
}
This would mean removing the first ??? comment. As for the second ???
comment, the answer is easy: because that's pretty much what defines a
bitfield! The size is DECL_SIZE_UNIT and not TYPE_SIZE_UNIT for them.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: RFC/A: set_mem_attributes_minus_bitpos tweak
2012-11-07 9:54 ` Eric Botcazou
@ 2012-11-15 19:50 ` Richard Sandiford
2012-11-15 23:10 ` Eric Botcazou
0 siblings, 1 reply; 4+ messages in thread
From: Richard Sandiford @ 2012-11-15 19:50 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Eric Botcazou <ebotcazou@adacore.com> writes:
>> expand_assignment calls:
>>
>> if (MEM_P (to_rtx))
>> {
>> /* If the field is at offset zero, we could have been given
>> the DECL_RTX of the parent struct. Don't munge it. */ to_rtx =
>> shallow_copy_rtx (to_rtx);
>>
>> set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
>> ...
>
> The MEM_KEEP_ALIAS_SET_P line seems to be redundant here (but not the
> MEM_VOLATILE_P line).
Good catch. I've removed that in the patch below.
>> But set_mem_attributes_minus_bitpos has:
>>
>> /* Default values from pre-existing memory attributes if present. */
>> refattrs = MEM_ATTRS (ref);
>> if (refattrs)
>> {
>> /* ??? Can this ever happen? Calling this routine on a MEM that
>> already carries memory attributes should probably be invalid. */
>> attrs.expr = refattrs->expr;
>> attrs.offset_known_p = refattrs->offset_known_p;
>> attrs.offset = refattrs->offset;
>> attrs.size_known_p = refattrs->size_known_p;
>> attrs.size = refattrs->size;
>> attrs.align = refattrs->align;
>> }
>>
>> which of course applies in this case: we have a MEM for g__style.
>> I would expect many calls from this site are for MEMs with an
>> existing MEM_EXPR.
>
> Indeed. Not very clear what to do though.
>
>> Then later:
>>
>> /* If this is a field reference and not a bit-field, record it. */
>> /* ??? There is some information that can be gleaned from bit-fields,
>> such as the word offset in the structure that might be modified.
>> But skip it for now. */
>> else if (TREE_CODE (t) == COMPONENT_REF
>> && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
>>
>> so we leave the offset and expr alone. The end result is:
>>
>> (mem/j/c:SI (symbol_ref:DI ("g__style") [flags 0x4] <var_decl
>> 0x7ffff6e4ee40 g__style>) [0 g__style+0 S1 A64])
>>
>> an SImode reference to the first byte (and only the first byte) of g__style.
>> Then, when we apply adjust_bitfield_address, it looks like we're moving
>> past the end of the region and so we drop the MEM_EXPR.
>>
>> In cases where set_mem_attributes_minus_bitpos does set MEM_EXPR based
>> on the new tree, it also adds the bitpos to the size. But I think we
>> should do that whenever we set the size based on the new tree,
>> regardless of whether we were able to record a MEM_EXPR too.
>>
>> That said, this code has lots of ???s in it, so I'm not exactly
>> confident about this change. Thoughts?
>
> It also seems a little bold to me. Since we now have the new processing of
> MEM_EXPR for bitfields, can't we remove the ! DECL_BIT_FIELD check?
>
> /* If this is a field reference and not a bit-field, record it. */
> /* ??? There is some information that can be gleaned from bit-fields,
> such as the word offset in the structure that might be modified.
> But skip it for now. */
> else if (TREE_CODE (t) == COMPONENT_REF
> && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
> {
> attrs.expr = t;
> attrs.offset_known_p = true;
> attrs.offset = 0;
> apply_bitpos = bitpos;
> /* ??? Any reason the field size would be different than
> the size we got from the type? */
> }
>
> This would mean removing the first ??? comment. As for the second ???
> comment, the answer is easy: because that's pretty much what defines a
> bitfield! The size is DECL_SIZE_UNIT and not TYPE_SIZE_UNIT for them.
I don't know this area well, so I'll have to take your word for it that
handling DECL_BIT_FIELDs is now safe. I certainly won't argue against
removing two ??? comments though :-)
Tested on x86_64-linux-gnu. OK to install?
Richard
gcc/
* expr.c (expand_assignment): Don't set MEM_KEEP_ALIAS_SET_P here.
* emit-rtl.c (set_mem_attributes_minus_bitpos): Handle DECL_BIT_FIELDs,
using their size instead of the COMPONENT_REF's.
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2012-11-10 22:17:24.797602770 +0000
+++ gcc/emit-rtl.c 2012-11-10 22:19:17.779228832 +0000
@@ -1679,11 +1679,7 @@ set_mem_attributes_minus_bitpos (rtx ref
attrs.align = MAX (attrs.align, TYPE_ALIGN (type));
/* If the size is known, we can set that. */
- if (TYPE_SIZE_UNIT (type) && host_integerp (TYPE_SIZE_UNIT (type), 1))
- {
- attrs.size_known_p = true;
- attrs.size = tree_low_cst (TYPE_SIZE_UNIT (type), 1);
- }
+ tree new_size = TYPE_SIZE_UNIT (type);
/* If T is not a type, we may be able to deduce some more information about
the expression. */
@@ -1742,13 +1738,7 @@ set_mem_attributes_minus_bitpos (rtx ref
attrs.offset_known_p = true;
attrs.offset = 0;
apply_bitpos = bitpos;
- if (DECL_SIZE_UNIT (t) && host_integerp (DECL_SIZE_UNIT (t), 1))
- {
- attrs.size_known_p = true;
- attrs.size = tree_low_cst (DECL_SIZE_UNIT (t), 1);
- }
- else
- attrs.size_known_p = false;
+ new_size = DECL_SIZE_UNIT (t);
attrs.align = DECL_ALIGN (t);
align_computed = true;
}
@@ -1763,19 +1753,15 @@ set_mem_attributes_minus_bitpos (rtx ref
align_computed = true;
}
- /* If this is a field reference and not a bit-field, record it. */
- /* ??? There is some information that can be gleaned from bit-fields,
- such as the word offset in the structure that might be modified.
- But skip it for now. */
- else if (TREE_CODE (t) == COMPONENT_REF
- && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
+ /* If this is a field reference, record it. */
+ else if (TREE_CODE (t) == COMPONENT_REF)
{
attrs.expr = t;
attrs.offset_known_p = true;
attrs.offset = 0;
apply_bitpos = bitpos;
- /* ??? Any reason the field size would be different than
- the size we got from the type? */
+ if (DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
+ new_size = DECL_SIZE_UNIT (TREE_OPERAND (t, 1));
}
/* If this is an array reference, look for an outer field reference. */
@@ -1861,6 +1847,12 @@ set_mem_attributes_minus_bitpos (rtx ref
else
as = TYPE_ADDR_SPACE (type);
+ if (host_integerp (new_size, 1))
+ {
+ attrs.size_known_p = true;
+ attrs.size = tree_low_cst (new_size, 1);
+ }
+
/* If we modified OFFSET based on T, then subtract the outstanding
bit position offset. Similarly, increase the size of the accessed
object to contain the negative offset. */
Index: gcc/expr.c
===================================================================
--- gcc/expr.c 2012-11-10 22:19:14.000000000 +0000
+++ gcc/expr.c 2012-11-10 22:21:17.615919793 +0000
@@ -4821,8 +4821,6 @@ expand_assignment (tree to, tree from, b
done for MEM. Also set MEM_KEEP_ALIAS_SET_P if needed. */
if (volatilep)
MEM_VOLATILE_P (to_rtx) = 1;
- if (component_uses_parent_alias_set (to))
- MEM_KEEP_ALIAS_SET_P (to_rtx) = 1;
}
if (optimize_bitfield_assignment_op (bitsize, bitpos,
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: RFC/A: set_mem_attributes_minus_bitpos tweak
2012-11-15 19:50 ` Richard Sandiford
@ 2012-11-15 23:10 ` Eric Botcazou
0 siblings, 0 replies; 4+ messages in thread
From: Eric Botcazou @ 2012-11-15 23:10 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> Tested on x86_64-linux-gnu. OK to install?
>
> Richard
>
>
> gcc/
> * expr.c (expand_assignment): Don't set MEM_KEEP_ALIAS_SET_P here.
> * emit-rtl.c (set_mem_attributes_minus_bitpos): Handle DECL_BIT_FIELDs,
> using their size instead of the COMPONENT_REF's.
Fine with me.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-11-15 23:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-03 11:53 RFC/A: set_mem_attributes_minus_bitpos tweak Richard Sandiford
2012-11-07 9:54 ` Eric Botcazou
2012-11-15 19:50 ` Richard Sandiford
2012-11-15 23:10 ` Eric Botcazou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).