* cprop_reg problem on sparc
@ 2011-10-27 12:06 David Miller
2011-10-27 18:21 ` Eric Botcazou
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2011-10-27 12:06 UTC (permalink / raw)
To: gcc
Although copy_value() in regcprop.c tries to avoid recording cases
where substitutions would be illegal, there are some bad cases it
still can let through.
On 64-bit sparc, integer regs are 64-bit and float regs are
(basically) 32-bit. So HARD_REGNO_NREGS(float_reg, DFmode) is 2, and
HARD_REGNO_NREGS(integer_reg, DImode) is 1.
cprop sees the sequence:
(insn 330 172 230 .. (set (reg:DI %g2) const_int)
(insn 171 330 173 .. (set (reg:DF %f10) (reg:DF %g2)))
(insn 173 171 222 .. (set (reg:DF %f2) (reg:DF %f10)))
(insn 222 173 223 .. (set (MEM:SI ..) (reg:SI %f10)))
(insn 223 222 174 .. (set (MEM:SI ..) (reg:SI %f11)))
And then it believes that in insn 222 it can replace %f10 with %g2,
but this is not a correct transformation.
cprop uses hard_regno_nregs[][] to attempt to detect illegal cases
like this one, but such checks will not trigger here because
hard_regno_nregs[][] is '1' for all of the registers being inspected:
hard_regno_nregs[][] (reg:SI f10) 1
hard_regno_nregs[][] (reg:DI g2) 1
The (set (reg:DI %g2) const_int) is generated by the *movdf_insn_sp64 insn
which in turn triggers a splitter for loading float constants into
integer registers.
The MEM:SI stores are reloads generated by IRA for a pseudo that has
to live across a call. For whatever reason it allocated only a 4-byte
aligned stack location, and I suppose that is why the reload is split
into 2 SImode pieces.
To reproduce build gcc.c-torture/execute/ieee/mzero.c with
"-m64 -mcpu=niagara3 -O2" on sparc.
I'm suspecting that perhaps cprop is ok, and the real issue is that
sparc's definition of CANNOT_CHANGE_MODE_CLASS needs to be adjusted.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: cprop_reg problem on sparc
2011-10-27 12:06 cprop_reg problem on sparc David Miller
@ 2011-10-27 18:21 ` Eric Botcazou
2011-10-27 19:36 ` David Miller
2011-10-27 21:56 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Eric Botcazou @ 2011-10-27 18:21 UTC (permalink / raw)
To: David Miller; +Cc: gcc
> On 64-bit sparc, integer regs are 64-bit and float regs are
> (basically) 32-bit. So HARD_REGNO_NREGS(float_reg, DFmode) is 2, and
> HARD_REGNO_NREGS(integer_reg, DImode) is 1.
>
> cprop sees the sequence:
>
> (insn 330 172 230 .. (set (reg:DI %g2) const_int)
> (insn 171 330 173 .. (set (reg:DF %f10) (reg:DF %g2)))
> (insn 173 171 222 .. (set (reg:DF %f2) (reg:DF %f10)))
> (insn 222 173 223 .. (set (MEM:SI ..) (reg:SI %f10)))
> (insn 223 222 174 .. (set (MEM:SI ..) (reg:SI %f11)))
>
> And then it believes that in insn 222 it can replace %f10 with %g2,
> but this is not a correct transformation.
>
> cprop uses hard_regno_nregs[][] to attempt to detect illegal cases
> like this one, but such checks will not trigger here because
> hard_regno_nregs[][] is '1' for all of the registers being inspected:
>
> hard_regno_nregs[][] (reg:SI f10) 1
> hard_regno_nregs[][] (reg:DI g2) 1
There seems to be a hole in the checks, as the number of registers is 2 for
some of the intermediate steps.
> To reproduce build gcc.c-torture/execute/ieee/mzero.c with
> "-m64 -mcpu=niagara3 -O2" on sparc.
AFAICS there is no such file as gcc.c-torture/execute/ieee/mzero.c.
> I'm suspecting that perhaps cprop is ok, and the real issue is that
> sparc's definition of CANNOT_CHANGE_MODE_CLASS needs to be adjusted.
I'm a little skeptical at this point.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: cprop_reg problem on sparc
2011-10-27 18:21 ` Eric Botcazou
@ 2011-10-27 19:36 ` David Miller
2011-10-27 22:42 ` Eric Botcazou
2011-10-27 21:56 ` David Miller
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2011-10-27 19:36 UTC (permalink / raw)
To: ebotcazou; +Cc: gcc
From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 27 Oct 2011 15:17:40 +0200
>> To reproduce build gcc.c-torture/execute/ieee/mzero.c with
>> "-m64 -mcpu=niagara3 -O2" on sparc.
>
> AFAICS there is no such file as gcc.c-torture/execute/ieee/mzero.c.
Sorry, the final path component should be "mzero2.c"
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: cprop_reg problem on sparc
2011-10-27 18:21 ` Eric Botcazou
2011-10-27 19:36 ` David Miller
@ 2011-10-27 21:56 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: David Miller @ 2011-10-27 21:56 UTC (permalink / raw)
To: ebotcazou; +Cc: gcc
From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 27 Oct 2011 15:17:40 +0200
>> On 64-bit sparc, integer regs are 64-bit and float regs are
>> (basically) 32-bit. So HARD_REGNO_NREGS(float_reg, DFmode) is 2, and
>> HARD_REGNO_NREGS(integer_reg, DImode) is 1.
>>
>> cprop sees the sequence:
>>
>> (insn 330 172 230 .. (set (reg:DI %g2) const_int)
>> (insn 171 330 173 .. (set (reg:DF %f10) (reg:DF %g2)))
>> (insn 173 171 222 .. (set (reg:DF %f2) (reg:DF %f10)))
>> (insn 222 173 223 .. (set (MEM:SI ..) (reg:SI %f10)))
>> (insn 223 222 174 .. (set (MEM:SI ..) (reg:SI %f11)))
>>
>> And then it believes that in insn 222 it can replace %f10 with %g2,
>> but this is not a correct transformation.
>>
>> cprop uses hard_regno_nregs[][] to attempt to detect illegal cases
>> like this one, but such checks will not trigger here because
>> hard_regno_nregs[][] is '1' for all of the registers being inspected:
>>
>> hard_regno_nregs[][] (reg:SI f10) 1
>> hard_regno_nregs[][] (reg:DI g2) 1
>
> There seems to be a hole in the checks, as the number of registers is 2 for
> some of the intermediate steps.
I think cprop_reg can legitimately only consider the candidate read
replacement (reg:SI %f10) with the most recent store to it's
equivalent value (reg:DI %g2).
In fact, that's exactly what cprop_reg's core algorithm is :-)
The issue is how to test properly that accesses to a value in two
register accesses is equivalent after a move. This pass is currently
using hard_regno_nregs[][] and a paradoxical subreg test to achieve
that, but it's obviously not sufficient.
Note that it would actually be legal to make the transformation on
insn 223, replacing %f11 with %g2.
But I'll note that if IRA had properly spilled %f10 to the stack using
an 8-byte aligned stack slot and DFmode, we wouldn't even be having to
consider this situation.
I think cprop_reg should cope with this properly, but I also think that
it would be nice if we worked to minimize unnecessary mode changes like
those seen here. The const-->DFmode splitter in sparc.md is partly to
blame, as is IRA.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: cprop_reg problem on sparc
2011-10-27 19:36 ` David Miller
@ 2011-10-27 22:42 ` Eric Botcazou
2011-10-28 0:58 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2011-10-27 22:42 UTC (permalink / raw)
To: David Miller; +Cc: gcc
> Sorry, the final path component should be "mzero2.c"
Thanks. I think we that need the same treatment in:
/* If we are accessing SRC in some mode other that what we
set it in, make sure that the replacement is valid. */
if (mode != vd->e[regno].mode)
{
if (hard_regno_nregs[regno][mode]
> hard_regno_nregs[regno][vd->e[regno].mode])
goto no_move_special_case;
}
as:
/* If we are narrowing the input to a smaller number of hard regs,
and it is in big endian, we are really extracting a high part.
Since we generally associate a low part of a value with the value itself,
we must not do the same for the high part.
Note we can still get low parts for the same mode combination through
a two-step copy involving differently sized hard regs.
Assume hard regs fr* are 32 bits bits each, while r* are 64 bits each:
(set (reg:DI r0) (reg:DI fr0))
(set (reg:SI fr2) (reg:SI r0))
loads the low part of (reg:DI fr0) - i.e. fr1 - into fr2, while:
(set (reg:SI fr2) (reg:SI fr0))
loads the high part of (reg:DI fr0) into fr2.
We can't properly represent the latter case in our tables, so don't
record anything then. */
else if (sn < (unsigned int) hard_regno_nregs[sr][vd->e[sr].mode]
&& (GET_MODE_SIZE (vd->e[sr].mode) > UNITS_PER_WORD
? WORDS_BIG_ENDIAN : BYTES_BIG_ENDIAN))
return;
i.e. we need to bail out if we are narrowing and this is a big-endian target.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: cprop_reg problem on sparc
2011-10-27 22:42 ` Eric Botcazou
@ 2011-10-28 0:58 ` David Miller
2011-10-28 2:17 ` Eric Botcazou
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2011-10-28 0:58 UTC (permalink / raw)
To: ebotcazou; +Cc: gcc
From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 27 Oct 2011 23:11:33 +0200
>> Sorry, the final path component should be "mzero2.c"
>
> Thanks. I think we that need the same treatment in:
...
> as:
...
> i.e. we need to bail out if we are narrowing and this is a big-endian target.
I quickly tried the patch below, but this does not prevent the
transformation.
diff --git a/gcc/regcprop.c b/gcc/regcprop.c
index ad92a64..54e008b 100644
--- a/gcc/regcprop.c
+++ b/gcc/regcprop.c
@@ -448,6 +448,14 @@ find_oldest_value_reg (enum reg_class cl, rtx reg, struct value_data *vd)
if (hard_regno_nregs[regno][mode]
> hard_regno_nregs[regno][vd->e[regno].mode])
return NULL_RTX;
+
+ /* And likewise, if we are narrowing on big endian the transformation
+ is also invalid. */
+ if (hard_regno_nregs[regno][mode]
+ < hard_regno_nregs[regno][vd->e[regno].mode]
+ && (GET_MODE_SIZE (vd->e[regno].mode) > UNITS_PER_WORD
+ ? WORDS_BIG_ENDIAN : BYTES_BIG_ENDIAN))
+ return NULL_RTX;
}
for (i = vd->e[regno].oldest_regno; i != regno; i = vd->e[i].next_regno)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: cprop_reg problem on sparc
2011-10-28 0:58 ` David Miller
@ 2011-10-28 2:17 ` Eric Botcazou
2011-10-28 5:06 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2011-10-28 2:17 UTC (permalink / raw)
To: David Miller; +Cc: gcc
> I quickly tried the patch below, but this does not prevent the
> transformation.
The quoted code is in copyprop_hardreg_forward_1.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: cprop_reg problem on sparc
2011-10-28 2:17 ` Eric Botcazou
@ 2011-10-28 5:06 ` David Miller
0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2011-10-28 5:06 UTC (permalink / raw)
To: ebotcazou; +Cc: gcc
From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 27 Oct 2011 23:55:00 +0200
>> I quickly tried the patch below, but this does not prevent the
>> transformation.
>
> The quoted code is in copyprop_hardreg_forward_1.
Indeed :-)
This patch below works for the specific test case, and I'll post to
gcc-patches and commit it after regstrapping.
Thanks Eric!
--------------------
Fix illegal register substitutions on big-endian during cprop_reg.
* regcprop.c (copyprop_hardreg_forward_1): Reject the
transformation when we narrow the mode on big endian.
---
gcc/ChangeLog | 5 +++++
gcc/regcprop.c | 8 ++++++++
2 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 403fb60..54e059e 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2011-10-27 David S. Miller <davem@davemloft.net>
+
+ * regcprop.c (copyprop_hardreg_forward_1): Reject the
+ transformation when we narrow the mode on big endian.
+
2011-10-27 Jakub Jelinek <jakub@redhat.com>
* config/i386/sse.md (avx_cvtpd2dq256_2, avx_cvttpd2dq256_2,
diff --git a/gcc/regcprop.c b/gcc/regcprop.c
index ad92a64..b0f0343 100644
--- a/gcc/regcprop.c
+++ b/gcc/regcprop.c
@@ -824,6 +824,14 @@ copyprop_hardreg_forward_1 (basic_block bb, struct value_data *vd)
if (hard_regno_nregs[regno][mode]
> hard_regno_nregs[regno][vd->e[regno].mode])
goto no_move_special_case;
+
+ /* And likewise, if we are narrowing on big endian the transformation
+ is also invalid. */
+ if (hard_regno_nregs[regno][mode]
+ < hard_regno_nregs[regno][vd->e[regno].mode]
+ && (GET_MODE_SIZE (vd->e[regno].mode) > UNITS_PER_WORD
+ ? WORDS_BIG_ENDIAN : BYTES_BIG_ENDIAN))
+ goto no_move_special_case;
}
/* If the destination is also a register, try to find a source
--
1.7.6.401.g6a319
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-10-28 3:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-27 12:06 cprop_reg problem on sparc David Miller
2011-10-27 18:21 ` Eric Botcazou
2011-10-27 19:36 ` David Miller
2011-10-27 22:42 ` Eric Botcazou
2011-10-28 0:58 ` David Miller
2011-10-28 2:17 ` Eric Botcazou
2011-10-28 5:06 ` David Miller
2011-10-27 21:56 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).