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