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.
