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