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

Reply via email to