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.
