On Fri, Sep 6, 2013 at 10:35 AM, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
> Richard,
>
>> But the movmisalign path skips all this code and with the
>> current code thinks the actual access is in the mode of the
>> whole structure. (and also misses the address adjustment
>> as shown in the other testcase for the bug)
>>
>> The movmisalign handling in this path is just broken. And
>> it's _not_ just for optimization! If you have
>>
>> struct __attribute__((packed)) {
>> double x;
>> v2df y;
>> } *p;
>>
>> and do
>>
>> p = malloc (48); // gets you 8-byte aligned memory
>> p->y = (v2df) { 1., 2. };
>>
>> then you cannot skip the movmisaling handling because
>> we'd generate an aligned move (based on the mode) then.
>>
>
> Ok, test examples are really helpful here.
>
> This time the structure is BLKmode, unaligned,
> movmisalign = false anyway.
>
> I tried to make a test case out of your example,
> and as I expected, the code is also correct:
>
> foo:
> .cfi_startproc
> movdqa .LC1(%rip), %xmm0
> movq $-1, (%rdi)
> movl $0x4048f5c3, 8(%rdi)
> movdqu %xmm0, 12(%rdi)
> ret
>
> movdqu.
>
> The test executes without trap.
> And I did everything to make the object unaligned.
Yeah, well - for the expand path with BLKmode it's working fine.
We're doing all
the dances because of correctness for non-BLKmode expand paths.
> I am sure we could completely remove the
> movmisalign path, and nothing would happen.
> probably. except maybe for a performance regression.
In this place probably yes. But we can also fix it to use the proper mode and
properly do the offset adjustment. But just adding a bounds check looks
broken to me.
Note that there may have been a correctness reason for this code in the
light of the IPA-SRA code. Maybe Martin remembers.
If removing the movmisalign handling inside the handled-component
case in expand_assignment works out (make sure to also test a
strict-alignment target) then that is probably fine.
Richard.
>
> Bernd.
