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

[Richard Biener <rguenther at suse dot de>]

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

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

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

Any ideas?

Thanks,
Richard.