On Tue, 10 Sep 2013 21:32:29, Martin Jambor wrote:
> Hi,
>
> On Fri, Sep 06, 2013 at 11:19:18AM +0200, Richard Biener wrote:
>> 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.
>>
>
> The misalignp path was added by you during the 4.7 development to fix
> PR 50444 which was indeed about expansion of a SRA generated statement
>
> MEM[(struct Engine *)e_1(D) + 40B].m = SR.18_17;
>
> If I disable this path on the 4.7 branch, the testcase is compiled
> incorrectly and aborts when run, apparently at least the 4.7's
> combination of expand_normal and store_field cannot cope with it.
>
> The path no longer tests the testcase though, because the component
> ref is not present in trunk, the LHS is now just
>
> MEM[(struct Engine *)e_3(D) + 40B]
>
> and so it is now handled just fine by the misaligned mem-ref case at
> the beginning of expand_assignment.I tried to remove the misaligned mem-ref case at the beginning of the expand_assignment, just to see how it fails. I did this on trunk. But still all testcases pass, including pr50444.c... How can this be? >> 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. >> > > I think we'd also better check that we do have a test where we expand > a COMPONENT_REF encapsulating a misaligned MEM_REF and a misaligned > MEM_REF that is mem_ref_refers_to_non_mem_p. > > I'm now going through all the new comments in bugzilla and the > testcases to see if I can still be of any help. > > Martin
