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.
>
