Re: Do not drom MEM_EXPR when accessing structure fields
On 09/19/16 16:23, Richard Biener wrote:
> On 19/09/16 15:49, Bernd Edlinger wrote:
>> On 09/19/16 11:28, Richard Biener wrote:
>>> On Sun, 18 Sep 2016, Jan Hubicka wrote:
>>>
> Hi,
>
>> when expanding code for
>> struct a {short a,b,c,d;} *a;
>> a->c;
>>
>> we first produce correct BLKmode MEM rtx representing the whole
>> structure *a,
>> then we use adjust_address to turn it into HImode MEM and finally
>> extract_bitfield is used to offset it by 32 bits to get correct
> value.
>
> I tried to create a test case as follows and stepped thru the code.
>
> cat test.c
> struct a {short a,b,c,d;};
> void foo (struct a *a)
> {
> a->c = 1;
> }
>
>
> First I get a DImode MEM rtx not BLKmode:
>
> (mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])
>
> and adjust_address does not clear the MEM_EXPR
>>>
>>> it's only cleared when later offsetted
>>>
> thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:
>
> (mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])
>
> and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:
>
> (mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
>
> which looks perfectly OK.
>>>
>>> Sure you quoted the correct RTL? MEM_SIZE == 6 looks wrong. Note
>>> we don't do set_mem_attributes_minus_bitpos when we go the bitfield
>>> extraction path.
>>>
>>
>> I think it is correct, to_rtx points 4 bytes before a->c "+-4", and
>> sizeof(a->c) is 2. So 4+2 is 6 there will be no access beyond that.
>
> Hmm, but if the MEM is HImode then doesn't this imply it will access
> 2 bytes starting at to_rtx (aka a->c "+-4"). At this point the offset
> doesn't seem to be accounted for yet (thus the MEM isn't accessing a->c
> yet).
>
Yes here to_rtx points still at a, and the MEM_EXPR means >c - 4b,
when the constant offset of 4 bytes gets added later in store_field,
and also there it looks like everything is working correctly.
6813 && TREE_CODE (exp) == MEM_REF
(gdb)
6770 if (mode == VOIDmode
(gdb)
6939 rtx to_rtx = adjust_address (target, mode, bitpos /
BITS_PER_UNIT);
(gdb) call debug(target)
(mem:HI (reg/v/f:DI 87 [ a ]) [2 a_2(D)->c+-4 S6 A16])
(gdb) p mode
$1 = HImode
(gdb) p bitpos
$2 = 32
(gdb) n
6941 if (to_rtx == target)
(gdb) call debug(to_rtx)
(mem:HI (plus:DI (reg/v/f:DI 87 [ a ])
(const_int 4 [0x4])) [2 a_2(D)->c+0 S2 A16])
(gdb)
>> And yes the RTL is correct: this is what I did:
>>
>> (gdb) n
>> 5152 to_rtx = shallow_copy_rtx (to_rtx);
>> (gdb) call debug(to_rtx)
>> (mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
>> (gdb) n
>> 5153 set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
>> (gdb) call debug(to_rtx)
>> (mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
>> (gdb) n
>> 5154 if (volatilep)
>> (gdb) call debug(to_rtx)
>> (mem:HI (reg/v/f:DI 87 [ a ]) [2 a_2(D)->c+-4 S6 A16])
>> (gdb)
>
> So set_mem_attributes_minus_bitpos seems to try making later adjustment
> by bitpos "valid" if we at the same time shrink size to what was
> originally intended.
>
>> It is not about bitfield extraction, as we are in expand_assignment.
>> next is the call to optimize_bitfield_assignment_op and
>> store_field.
>>
>> I believe, that with Jan's patch we have at this point only
>> a different mode on the to_rtx. And it is possible that
>> store_field or store_bit_field takes a different decision
>> dependent on GET_MODE(to_rtx), which may accidentally delete
>> the MEM_EXPR.
>
> It offsets the MEM which causes it to go "out of bounds" (no such
> "fancy" too large MEM_SIZE is present) which causes us to drop the
> MEM_EXPR.
>
> IMHO this piecewise (re-)construction of MEM_ATTRS is quite fragile...
> But refactoring this area of the compiler is fragile as well...
>
Yes...
Bernd.
Re: Do not drom MEM_EXPR when accessing structure fields
On 19/09/16 15:49, Bernd Edlinger wrote:
> On 09/19/16 11:28, Richard Biener wrote:
>> On Sun, 18 Sep 2016, Jan Hubicka wrote:
>>
Hi,
> when expanding code for
> struct a {short a,b,c,d;} *a;
> a->c;
>
> we first produce correct BLKmode MEM rtx representing the whole
> structure *a,
> then we use adjust_address to turn it into HImode MEM and finally
> extract_bitfield is used to offset it by 32 bits to get correct value.
I tried to create a test case as follows and stepped thru the code.
cat test.c
struct a {short a,b,c,d;};
void foo (struct a *a)
{
a->c = 1;
}
First I get a DImode MEM rtx not BLKmode:
(mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])
and adjust_address does not clear the MEM_EXPR
>>
>> it's only cleared when later offsetted
>>
thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:
(mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])
and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:
(mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
which looks perfectly OK.
>>
>> Sure you quoted the correct RTL? MEM_SIZE == 6 looks wrong. Note
>> we don't do set_mem_attributes_minus_bitpos when we go the bitfield
>> extraction path.
>>
>
> I think it is correct, to_rtx points 4 bytes before a->c "+-4", and
> sizeof(a->c) is 2. So 4+2 is 6 there will be no access beyond that.
Hmm, but if the MEM is HImode then doesn't this imply it will access
2 bytes starting at to_rtx (aka a->c "+-4"). At this point the offset
doesn't seem to be accounted for yet (thus the MEM isn't accessing a->c
yet).
> And yes the RTL is correct: this is what I did:
>
> (gdb) n
> 5152to_rtx = shallow_copy_rtx (to_rtx);
> (gdb) call debug(to_rtx)
> (mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
> (gdb) n
> 5153set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
> (gdb) call debug(to_rtx)
> (mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
> (gdb) n
> 5154if (volatilep)
> (gdb) call debug(to_rtx)
> (mem:HI (reg/v/f:DI 87 [ a ]) [2 a_2(D)->c+-4 S6 A16])
> (gdb)
So set_mem_attributes_minus_bitpos seems to try making later adjustment
by bitpos "valid" if we at the same time shrink size to what was
originally intended.
> It is not about bitfield extraction, as we are in expand_assignment.
> next is the call to optimize_bitfield_assignment_op and
> store_field.
>
> I believe, that with Jan's patch we have at this point only
> a different mode on the to_rtx. And it is possible that
> store_field or store_bit_field takes a different decision
> dependent on GET_MODE(to_rtx), which may accidentally delete
> the MEM_EXPR.
It offsets the MEM which causes it to go "out of bounds" (no such
"fancy" too large MEM_SIZE is present) which causes us to drop the
MEM_EXPR.
IMHO this piecewise (re-)construction of MEM_ATTRS is quite fragile...
But refactoring this area of the compiler is fragile as well...
Richard.
>
> Bernd.
>
But with your patch the RTX looks different:
(mem:DI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
which looks inconsistent, and not like an improvement.
>>>
>>> Hmm,
>>> the memory reference does point to a_2(D)->c+-4 so I would say it is OK.
>>> The
>>> memory refernece is not adjusted yet to be reg87+4 and this is where memory
>>> attributes got lost for me (because the pointer becames out of range of
>>> (mem:HI
>>> (reg87))). I see this does not happen on x86_64, I will try to see why
>>> tomorrow.
>>
>> I think if you don't go the bitfield path nothing ends up chaning the
>> mode. The inconsistency is for MEM_SIZE vs. GET_MODE of the MEM.
>>
>> mem:DI certainly looks wrong for a HImode access.
>>
>> Richard.
>>
>
Re: Do not drom MEM_EXPR when accessing structure fields
On 09/19/16 11:28, Richard Biener wrote:
> On Sun, 18 Sep 2016, Jan Hubicka wrote:
>
>>> Hi,
>>>
>>> > when expanding code for
>>> > struct a {short a,b,c,d;} *a;
>>> > a->c;
>>> >
>>> > we first produce correct BLKmode MEM rtx representing the whole
>>> > structure *a,
>>> > then we use adjust_address to turn it into HImode MEM and finally
>>> > extract_bitfield is used to offset it by 32 bits to get correct value.
>>>
>>> I tried to create a test case as follows and stepped thru the code.
>>>
>>> cat test.c
>>> struct a {short a,b,c,d;};
>>> void foo (struct a *a)
>>> {
>>> a->c = 1;
>>> }
>>>
>>>
>>> First I get a DImode MEM rtx not BLKmode:
>>>
>>> (mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])
>>>
>>> and adjust_address does not clear the MEM_EXPR
>
> it's only cleared when later offsetted
>
>>> thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:
>>>
>>> (mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])
>>>
>>> and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:
>>>
>>> (mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
>>>
>>> which looks perfectly OK.
>
> Sure you quoted the correct RTL? MEM_SIZE == 6 looks wrong. Note
> we don't do set_mem_attributes_minus_bitpos when we go the bitfield
> extraction path.
>
I think it is correct, to_rtx points 4 bytes before a->c "+-4", and
sizeof(a->c) is 2. So 4+2 is 6 there will be no access beyond that.
And yes the RTL is correct: this is what I did:
(gdb) n
5152 to_rtx = shallow_copy_rtx (to_rtx);
(gdb) call debug(to_rtx)
(mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
(gdb) n
5153 set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
(gdb) call debug(to_rtx)
(mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
(gdb) n
5154 if (volatilep)
(gdb) call debug(to_rtx)
(mem:HI (reg/v/f:DI 87 [ a ]) [2 a_2(D)->c+-4 S6 A16])
(gdb)
It is not about bitfield extraction, as we are in expand_assignment.
next is the call to optimize_bitfield_assignment_op and
store_field.
I believe, that with Jan's patch we have at this point only
a different mode on the to_rtx. And it is possible that
store_field or store_bit_field takes a different decision
dependent on GET_MODE(to_rtx), which may accidentally delete
the MEM_EXPR.
Bernd.
>>>
>>> But with your patch the RTX looks different:
>>>
>>> (mem:DI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
>>>
>>> which looks inconsistent, and not like an improvement.
>>
>> Hmm,
>> the memory reference does point to a_2(D)->c+-4 so I would say it is OK. The
>> memory refernece is not adjusted yet to be reg87+4 and this is where memory
>> attributes got lost for me (because the pointer becames out of range of
>> (mem:HI
>> (reg87))). I see this does not happen on x86_64, I will try to see why
>> tomorrow.
>
> I think if you don't go the bitfield path nothing ends up chaning the
> mode. The inconsistency is for MEM_SIZE vs. GET_MODE of the MEM.
>
> mem:DI certainly looks wrong for a HImode access.
>
> Richard.
>
Re: Do not drom MEM_EXPR when accessing structure fields
On 09/18/2016 02:51 PM, Jan Hubicka wrote:
to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
-
- /* If the field has a mode, we want to access it in the
-field's mode, not the computed mode.
-If a MEM has VOIDmode (external with incomplete type),
-use BLKmode for it instead. */
- if (MEM_P (to_rtx))
- {
- if (mode1 != VOIDmode)
- to_rtx = adjust_address (to_rtx, mode1, 0);
- else if (GET_MODE (to_rtx) == VOIDmode)
- to_rtx = adjust_address (to_rtx, BLKmode, 0);
- }
if (offset != 0)
{
This was added by Jakub in r166603, referencing PR46388. Have you looked
at that?
Bernd
Re: Do not drom MEM_EXPR when accessing structure fields
On Sun, 18 Sep 2016, Jan Hubicka wrote:
> > Hi,
> >
> > > when expanding code for
> > > struct a {short a,b,c,d;} *a;
> > > a->c;
> > >
> > > we first produce correct BLKmode MEM rtx representing the whole
> > > structure *a,
> > > then we use adjust_address to turn it into HImode MEM and finally
> > > extract_bitfield is used to offset it by 32 bits to get correct value.
> >
> > I tried to create a test case as follows and stepped thru the code.
> >
> > cat test.c
> > struct a {short a,b,c,d;};
> > void foo (struct a *a)
> > {
> >a->c = 1;
> > }
> >
> >
> > First I get a DImode MEM rtx not BLKmode:
> >
> > (mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])
> >
> > and adjust_address does not clear the MEM_EXPR
it's only cleared when later offsetted
> > thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:
> >
> > (mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])
> >
> > and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:
> >
> > (mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
> >
> > which looks perfectly OK.
Sure you quoted the correct RTL? MEM_SIZE == 6 looks wrong. Note
we don't do set_mem_attributes_minus_bitpos when we go the bitfield
extraction path.
> >
> > But with your patch the RTX looks different:
> >
> > (mem:DI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
> >
> > which looks inconsistent, and not like an improvement.
>
> Hmm,
> the memory reference does point to a_2(D)->c+-4 so I would say it is OK. The
> memory refernece is not adjusted yet to be reg87+4 and this is where memory
> attributes got lost for me (because the pointer becames out of range of
> (mem:HI
> (reg87))). I see this does not happen on x86_64, I will try to see why
> tomorrow.
I think if you don't go the bitfield path nothing ends up chaning the
mode. The inconsistency is for MEM_SIZE vs. GET_MODE of the MEM.
mem:DI certainly looks wrong for a HImode access.
Richard.
Re: Do not drom MEM_EXPR when accessing structure fields
> Hi,
>
> > when expanding code for
> > struct a {short a,b,c,d;} *a;
> > a->c;
> >
> > we first produce correct BLKmode MEM rtx representing the whole
> > structure *a,
> > then we use adjust_address to turn it into HImode MEM and finally
> > extract_bitfield is used to offset it by 32 bits to get correct value.
>
> I tried to create a test case as follows and stepped thru the code.
>
> cat test.c
> struct a {short a,b,c,d;};
> void foo (struct a *a)
> {
>a->c = 1;
> }
>
>
> First I get a DImode MEM rtx not BLKmode:
>
> (mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])
>
> and adjust_address does not clear the MEM_EXPR
>
> thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:
>
>
> (mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])
>
> and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:
>
> (mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
>
> which looks perfectly OK.
>
> But with your patch the RTX looks different:
>
> (mem:DI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
>
> which looks inconsistent, and not like an improvement.
Hmm,
the memory reference does point to a_2(D)->c+-4 so I would say it is OK. The
memory refernece is not adjusted yet to be reg87+4 and this is where memory
attributes got lost for me (because the pointer becames out of range of (mem:HI
(reg87))). I see this does not happen on x86_64, I will try to see why
tomorrow.
Honza
Re: Do not drom MEM_EXPR when accessing structure fields
Hi,
> when expanding code for
> struct a {short a,b,c,d;} *a;
> a->c;
>
> we first produce correct BLKmode MEM rtx representing the whole
> structure *a,
> then we use adjust_address to turn it into HImode MEM and finally
> extract_bitfield is used to offset it by 32 bits to get correct value.
I tried to create a test case as follows and stepped thru the code.
cat test.c
struct a {short a,b,c,d;};
void foo (struct a *a)
{
a->c = 1;
}
First I get a DImode MEM rtx not BLKmode:
(mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])
and adjust_address does not clear the MEM_EXPR
thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:
(mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])
and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:
(mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
which looks perfectly OK.
But with your patch the RTX looks different:
(mem:DI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
which looks inconsistent, and not like an improvement.
Especially when the mode of the target_rtx may be used
somewhere in store_field:
/* If TEMP is not a PARALLEL (see below) and its mode and that of
TARGET
are both BLKmode, both must be in memory and BITPOS must be
aligned
on a byte boundary. If so, we simply do a block copy.
Likewise for
a BLKmode-like TARGET. */
if (GET_CODE (temp) != PARALLEL
&& GET_MODE (temp) == BLKmode
|| (MEM_P (target)
&& GET_MODE_CLASS (GET_MODE (target)) == MODE_INT
and I see store_fixed_bit_field also looks at the memory mode:
if (MEM_P (op0))
{
machine_mode mode = GET_MODE (op0);
if (GET_MODE_BITSIZE (mode) == 0
|| GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (word_mode))
mode = word_mode;
mode = get_best_mode (bitsize, bitnum, bitregion_start,
bitregion_end,
MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
Bernd.
Do not drom MEM_EXPR when accessing structure fields
Hi,
when expanding code for
struct a {short a,b,c,d;} *a;
a->c;
we first produce correct BLKmode MEM rtx representing the whole structure *a,
then we use adjust_address to turn it into HImode MEM and finally
extract_bitfield is used to offset it by 32 bits to get correct value.
The catch is that adjust_address updates memory attributes, too, and it
sets size to 2 and clears MEM_EXPR on this occasion. According to Richard
this is only correct behaviour because the size must represent size of memory
load (and thus has meaning only for BLKmodes).
This patch simply drops the original cast, because as far as I can tell
all the following paths that does handle MEM also will update the mode
once the location was correclty offseted. (It is bit hard to follow all
possible paths as it goes down to different ways of bitfield processing,
but if some path is missing and not tested by x86_64 bootstrap, we should
update it to change the mode later).
This significantly improves alignment tracking.
Bootstrapped/regstested x86_64-linux (including Ada), OK?
expr.c (expand_assignment): Do not change memory mode.
Index: expr.c
===
--- expr.c (revision 240109)
+++ expr.c (working copy)
@@ -5010,18 +5010,6 @@ expand_assignment (tree to, tree from, b
}
to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
-
- /* If the field has a mode, we want to access it in the
-field's mode, not the computed mode.
-If a MEM has VOIDmode (external with incomplete type),
-use BLKmode for it instead. */
- if (MEM_P (to_rtx))
- {
- if (mode1 != VOIDmode)
- to_rtx = adjust_address (to_rtx, mode1, 0);
- else if (GET_MODE (to_rtx) == VOIDmode)
- to_rtx = adjust_address (to_rtx, BLKmode, 0);
- }
if (offset != 0)
{
