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