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
