Patchwork [PING,PR,57748] Set mode of structures with zero sized arrays to be BLK

login
register
mail settings
Submitter Richard Guenther
Date Aug. 28, 2013, 8:17 a.m.
Message ID <alpine.LNX.2.00.1308281011340.20077@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/270385/
State New
Headers show

Comments

Richard Guenther - Aug. 28, 2013, 8:17 a.m.
On Wed, 28 Aug 2013, Richard Biener wrote:

> On Tue, 27 Aug 2013, Jakub Jelinek wrote:
> 
> > On Tue, Aug 27, 2013 at 04:03:42PM +0200, Martin Jambor wrote:
> > > On Fri, Aug 23, 2013 at 05:29:23PM +0200, Jakub Jelinek wrote:
> > > > On Fri, Aug 23, 2013 at 05:11:22PM +0200, Martin Jambor wrote:
> > > > > Hi Jakub and/or Joseph,
> > > > > 
> > > > > the reporter of this bug seems to be very anxious to have it fixed in
> > > > > the repository.  While Richi is away, do you think you could have a
> > > > > look?  It is very small.
> > > > 
> > > > Isn't this ABI incompatible change (at least potential on various targets)?
> > > > If so, then it shouldn't be applied to release branches, because it would
> > > > create (potential?) ABI incompatibility between 4.8.[01] and 4.8.2.
> > > > 
> > > 
> > > I don't know.  I did a few attempts to observe a change in the calling
> > > convention of a function accepting a zero sized array terminated
> > > structure by value (on x86_64) but I did not succeed.  On the other
> > > hand, I assume there are many other ways how a mode can influence ABI.
> > > So I'll try to have a look whether we can hack around this situation
> > > in 4.8's expr.c instead.
> > 
> > All I remember that e.g. the number of regressions from PR20020 was big,
> > and any kind of TYPE_MODE changes are just extremely risky.
> > Perhaps x86_64 in the ABI decisions never uses mode, but we have tons of
> > other targets and it would surprise me if none of those were affected.
> > 
> > > Nevertheless, is this patch ok for trunk?
> > 
> > I'll defer that to Richard now that he is back ;)
> 
> Eh ... :/
> 
> I'm extremely nervous about this change.  I also believe the change
> is unrelated to the issue in the bugreport (even if it happens to
> fix the ICE).
> 
> Let me have a (short) look.

Everything looks ok up until

      if (offset != 0)
        {
          enum machine_mode address_mode;

where we are supposed to factor in offset into the memory address.
This code doesn't handle the movmisalign path which has the memory
in 'mem' instead of in to_rtx.

And indeed a hack like

       /* No action is needed if the target is not a memory and the field

fixes the testcase and results in (at -O)

foo:
.LFB1:
        .cfi_startproc
        pushq   %rbx
        .cfi_def_cfa_offset 16
        .cfi_offset 3, -16
        movq    %rdi, %rbx
        call    get_i
        cltq
        addq    $1, %rax
        salq    $4, %rax
        movdqa  .LC0(%rip), %xmm0
        movdqu  %xmm0, (%rbx,%rax)
        popq    %rbx
        .cfi_def_cfa_offset 8
        ret

exactly what is expected.

Martin, do you want to audit the (few) places we do the movmisalign
game for the same problem or shall I put it on my TODO?  The audit
should look for all MEM_P (to_rtx) tests which really should look
at 'mem' for the unaligned move case (I see a volatile bitfiled
case for example ...).

Still need to figure a "nice" way to restructure the code ...

Any ideas?

Thanks,
Richard.
Martin Jambor - Aug. 28, 2013, 9:21 a.m.
On Wed, Aug 28, 2013 at 10:17:52AM +0200, Richard Biener wrote:
> On Wed, 28 Aug 2013, Richard Biener wrote:
> > Eh ... :/
> > 
> > I'm extremely nervous about this change.  I also believe the change
> > is unrelated to the issue in the bugreport (even if it happens to
> > fix the ICE).
> > 
> > Let me have a (short) look.
> 
> Everything looks ok up until
> 
>       if (offset != 0)
>         {
>           enum machine_mode address_mode;
> 
> where we are supposed to factor in offset into the memory address.
> This code doesn't handle the movmisalign path which has the memory
> in 'mem' instead of in to_rtx.
> 
> And indeed a hack like
> 
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c  (revision 202043)
> +++ gcc/expr.c  (working copy)
> @@ -4753,6 +4753,9 @@ expand_assignment (tree to, tree from, b
>         {
>           enum machine_mode address_mode;
>           rtx offset_rtx;
> +         rtx saved_to_rtx = to_rtx;
> +         if (misalignp)
> +           to_rtx = mem;
>  
>           if (!MEM_P (to_rtx))
>             {
> @@ -4785,6 +4788,11 @@ expand_assignment (tree to, tree from, b
>           to_rtx = offset_address (to_rtx, offset_rtx,
>                                    highest_pow2_factor_for_target (to,
>                                                                    
> offset));
> +         if (misalignp)
> +           {
> +             mem = to_rtx;
> +             to_rtx = saved_to_rtx;
> +           }
>         }
>  
>        /* No action is needed if the target is not a memory and the field
> 
> fixes the testcase and results in (at -O)
> 
> foo:
> .LFB1:
>         .cfi_startproc
>         pushq   %rbx
>         .cfi_def_cfa_offset 16
>         .cfi_offset 3, -16
>         movq    %rdi, %rbx
>         call    get_i
>         cltq
>         addq    $1, %rax
>         salq    $4, %rax
>         movdqa  .LC0(%rip), %xmm0
>         movdqu  %xmm0, (%rbx,%rax)
>         popq    %rbx
>         .cfi_def_cfa_offset 8
>         ret
> 
> exactly what is expected.
> 
> Martin, do you want to audit the (few) places we do the movmisalign
> game for the same problem or shall I put it on my TODO?  The audit
> should look for all MEM_P (to_rtx) tests which really should look
> at 'mem' for the unaligned move case (I see a volatile bitfiled
> case for example ...).

And I thought I had an easy way out.  Well, let me see what I can do.

Thanks,

Martin

Patch

Index: gcc/expr.c
===================================================================
--- gcc/expr.c  (revision 202043)
+++ gcc/expr.c  (working copy)
@@ -4753,6 +4753,9 @@  expand_assignment (tree to, tree from, b
        {
          enum machine_mode address_mode;
          rtx offset_rtx;
+         rtx saved_to_rtx = to_rtx;
+         if (misalignp)
+           to_rtx = mem;
 
          if (!MEM_P (to_rtx))
            {
@@ -4785,6 +4788,11 @@  expand_assignment (tree to, tree from, b
          to_rtx = offset_address (to_rtx, offset_rtx,
                                   highest_pow2_factor_for_target (to,
                                                                   
offset));
+         if (misalignp)
+           {
+             mem = to_rtx;
+             to_rtx = saved_to_rtx;
+           }
        }