* [PATCH] Fix ICE on user view-conversion (v2)
@ 2008-09-26 20:45 Eric Botcazou
2008-09-26 21:35 ` Richard Henderson
0 siblings, 1 reply; 10+ messages in thread
From: Eric Botcazou @ 2008-09-26 20:45 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]
Hi,
this is a follow-up to
http://gcc.gnu.org/ml/gcc-patches/2007-11/msg00248.html
which was installed by Olivier a few months ago.
It turns out that the testcase still fails on MIPS because the back-end
refuses to put the (legitimate) constant in the constant pool:
if (CONSTANT_P (op0))
{
[...]
else
op0 = validize_mem (force_const_mem (mode, op0));
}
that is, force_const_mem returns NULL_RTX because of:
static bool
mips_cannot_force_const_mem (rtx x)
{
enum mips_symbol_type type;
rtx base, offset;
[...]
/* As an optimization, reject constants that mips_legitimize_move
can expand inline.
Suppose we have a multi-instruction sequence that loads constant C
into register R. If R does not get allocated a hard register, and
R is used in an operand that allows both registers and memory
references, reload will consider forcing C into memory and using
one of the instruction's memory alternatives. Returning false
here will force it to use an input reload instead. */
if (GET_CODE (x) == CONST_INT && LEGITIMATE_CONSTANT_P (x))
return true;
The proposed fix is to fall back to the third case, i.e. to load the constant
into a register and then put it in (automatic) memory.
Tested on i586-suse-linux, OK for mainline?
2008-09-26 Eric Botcazou <ebotcazou@adacore.com>
* expr.c (expand_expr_real_1) <normal_inner_ref>: Force op0 to
non-constant memory if it cannot be forced to constant memory.
Overhaul surrounding code and factor out common condition.
--
Eric Botcazou
[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 4209 bytes --]
Index: expr.c
===================================================================
--- expr.c (revision 140680)
+++ expr.c (working copy)
@@ -7744,13 +7744,13 @@ expand_expr_real_1 (tree exp, rtx target
case ARRAY_RANGE_REF:
normal_inner_ref:
{
- enum machine_mode mode1;
+ enum machine_mode mode1, mode2;
HOST_WIDE_INT bitsize, bitpos;
tree offset;
- int volatilep = 0;
+ int volatilep = 0, must_force_mem;
tree tem = get_inner_reference (exp, &bitsize, &bitpos, &offset,
&mode1, &unsignedp, &volatilep, true);
- rtx orig_op0;
+ rtx orig_op0, memloc;
/* If we got back the original object, something is wrong. Perhaps
we are evaluating an expression too early. In any event, don't
@@ -7760,7 +7760,6 @@ expand_expr_real_1 (tree exp, rtx target
/* If TEM's type is a union of variable size, pass TARGET to the inner
computation, since it will need a temporary and TARGET is known
to have to do. This occurs in unchecked conversion in Ada. */
-
orig_op0 = op0
= expand_expr (tem,
(TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
@@ -7774,45 +7773,47 @@ expand_expr_real_1 (tree exp, rtx target
|| modifier == EXPAND_STACK_PARM)
? modifier : EXPAND_NORMAL);
- /* If this is a constant, put it into a register if it is a legitimate
- constant, OFFSET is 0, and we won't try to extract outside the
- register (in case we were passed a partially uninitialized object
- or a view_conversion to a larger size) or a BLKmode piece of it
- (e.g. if it is unchecked-converted to a record type in Ada). Force
- the constant to memory otherwise. */
- if (CONSTANT_P (op0))
- {
- enum machine_mode mode = TYPE_MODE (TREE_TYPE (tem));
- if (mode != BLKmode && LEGITIMATE_CONSTANT_P (op0)
- && offset == 0
- && mode1 != BLKmode
- && bitpos + bitsize <= GET_MODE_BITSIZE (mode))
- op0 = force_reg (mode, op0);
- else
- op0 = validize_mem (force_const_mem (mode, op0));
- }
+ mode2
+ = CONSTANT_P (op0) ? TYPE_MODE (TREE_TYPE (tem)) : GET_MODE (op0);
- /* Otherwise, if this object not in memory and we either have an
- offset, a BLKmode result, or a reference outside the object, put it
- there. Such cases can occur in Ada if we have unchecked conversion
- of an expression from a scalar type to an array or record type or
- for an ARRAY_RANGE_REF whose type is BLKmode. */
- else if (!MEM_P (op0)
- && (offset != 0
- || mode1 == BLKmode
- || (bitpos + bitsize
- > GET_MODE_BITSIZE (GET_MODE (op0)))))
+ /* If we have either an offset, a BLKmode result, or a reference
+ outside the underlying object, we must force it to memory.
+ Such a case can occur in Ada if we have unchecked conversion
+ of an expression from a scalar type to an aggregate type or
+ for an ARRAY_RANGE_REF whose type is BLKmode, or if we were
+ passed a partially uninitialized object or a view-conversion
+ to a larger size. */
+ must_force_mem = (offset
+ || mode1 == BLKmode
+ || bitpos + bitsize > GET_MODE_BITSIZE (mode2));
+
+ /* If this is a constant, put it in a register if it is a legitimate
+ constant and we don't need a memory reference. */
+ if (CONSTANT_P (op0)
+ && mode2 != BLKmode
+ && LEGITIMATE_CONSTANT_P (op0)
+ && !must_force_mem)
+ op0 = force_reg (mode2, op0);
+
+ /* Otherwise, if this is a constant, try to force it to the constant
+ pool. Note that back-ends, e.g. MIPS, may refuse to do so if it
+ is a legitimate constant. */
+ else if (CONSTANT_P (op0) && (memloc = force_const_mem (mode2, op0)))
+ op0 = validize_mem (memloc);
+
+ /* Otherwise, if this is a constant or the object is not in memory
+ and need be, put it there. */
+ else if (CONSTANT_P (op0) || (!MEM_P (op0) && must_force_mem))
{
tree nt = build_qualified_type (TREE_TYPE (tem),
(TYPE_QUALS (TREE_TYPE (tem))
| TYPE_QUAL_CONST));
- rtx memloc = assign_temp (nt, 1, 1, 1);
-
+ memloc = assign_temp (nt, 1, 1, 1);
emit_move_insn (memloc, op0);
op0 = memloc;
}
- if (offset != 0)
+ if (offset)
{
rtx offset_rtx = expand_expr (offset, NULL_RTX, VOIDmode,
EXPAND_SUM);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix ICE on user view-conversion (v2)
2008-09-26 20:45 [PATCH] Fix ICE on user view-conversion (v2) Eric Botcazou
@ 2008-09-26 21:35 ` Richard Henderson
2008-09-26 21:46 ` Andrew Pinski
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Richard Henderson @ 2008-09-26 21:35 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, bernd.schmidt, aoliva
Eric Botcazou wrote:
> /* As an optimization, reject constants that mips_legitimize_move
> can expand inline.
>
> Suppose we have a multi-instruction sequence that loads constant C
> into register R. If R does not get allocated a hard register, and
> R is used in an operand that allows both registers and memory
> references, reload will consider forcing C into memory and using
> one of the instruction's memory alternatives. Returning false
> here will force it to use an input reload instead. */
> if (GET_CODE (x) == CONST_INT && LEGITIMATE_CONSTANT_P (x))
> return true;
I think this is a bug in the mips port. And looking closer, in
the bfin and frv ports as well. Everyone else appears to actually
identify those constants that *can't* go to memory. In particular,
I don't see CONST_INT, CONST_DOUBLE, or CONST_VECTOR being rejected.
These 3 ports are all assuming that force_const_mem is only called
by reload to push non-legitimate constants to memory. There are
clearly other applications. This is one; there are several others.
Another option for these ports is if we add another parameter that
specifies that this is being called from reload. Though, knowing
reload there's probably a cleaner way to handle that.
r~
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix ICE on user view-conversion (v2)
2008-09-26 21:35 ` Richard Henderson
@ 2008-09-26 21:46 ` Andrew Pinski
2008-09-26 21:52 ` Richard Henderson
2008-09-26 22:02 ` Eric Botcazou
2008-09-28 1:19 ` Richard Sandiford
2 siblings, 1 reply; 10+ messages in thread
From: Andrew Pinski @ 2008-09-26 21:46 UTC (permalink / raw)
To: Richard Henderson; +Cc: Eric Botcazou, gcc-patches, bernd.schmidt, aoliva
On Fri, Sep 26, 2008 at 2:16 PM, Richard Henderson <rth@redhat.com> wrote:
> Another option for these ports is if we add another parameter that
> specifies that this is being called from reload. Though, knowing
> reload there's probably a cleaner way to handle that.
Won't reload_in_progress be true so the MIPS back-end could check that right?
Thanks,
Andrew Pinski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix ICE on user view-conversion (v2)
2008-09-26 21:46 ` Andrew Pinski
@ 2008-09-26 21:52 ` Richard Henderson
0 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2008-09-26 21:52 UTC (permalink / raw)
To: Andrew Pinski; +Cc: Eric Botcazou, gcc-patches, bernd.schmidt, aoliva
Andrew Pinski wrote:
> On Fri, Sep 26, 2008 at 2:16 PM, Richard Henderson <rth@redhat.com> wrote:
>> Another option for these ports is if we add another parameter that
>> specifies that this is being called from reload. Though, knowing
>> reload there's probably a cleaner way to handle that.
>
> Won't reload_in_progress be true so the MIPS back-end could check that right?
Yes, that's certainly a possibility.
r~
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix ICE on user view-conversion (v2)
2008-09-26 21:35 ` Richard Henderson
2008-09-26 21:46 ` Andrew Pinski
@ 2008-09-26 22:02 ` Eric Botcazou
2008-09-26 23:41 ` Richard Henderson
2008-09-28 1:19 ` Richard Sandiford
2 siblings, 1 reply; 10+ messages in thread
From: Eric Botcazou @ 2008-09-26 22:02 UTC (permalink / raw)
To: Richard Henderson; +Cc: gcc-patches, bernd.schmidt, aoliva
> I think this is a bug in the mips port. And looking closer, in
> the bfin and frv ports as well. Everyone else appears to actually
> identify those constants that *can't* go to memory. In particular,
> I don't see CONST_INT, CONST_DOUBLE, or CONST_VECTOR being rejected.
On the other hand, I can understand that a port really wants to avoid wasting
space by putting legitimate constants in the pool. Maybe force_const_mem
should simply not be invoked on legitimate constants at all.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix ICE on user view-conversion (v2)
2008-09-26 22:02 ` Eric Botcazou
@ 2008-09-26 23:41 ` Richard Henderson
2008-09-27 10:52 ` Eric Botcazou
0 siblings, 1 reply; 10+ messages in thread
From: Richard Henderson @ 2008-09-26 23:41 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, bernd.schmidt, aoliva
Eric Botcazou wrote:
>> I think this is a bug in the mips port. And looking closer, in
>> the bfin and frv ports as well. Everyone else appears to actually
>> identify those constants that *can't* go to memory. In particular,
>> I don't see CONST_INT, CONST_DOUBLE, or CONST_VECTOR being rejected.
>
> On the other hand, I can understand that a port really wants to avoid wasting
> space by putting legitimate constants in the pool. Maybe force_const_mem
> should simply not be invoked on legitimate constants at all.
So we're going to avoid putting one 4 byte constant into memory
in exchange for 12 bytes of instructions to put the constant
onto the stack? That seems a bit silly to me.
r~
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix ICE on user view-conversion (v2)
2008-09-26 23:41 ` Richard Henderson
@ 2008-09-27 10:52 ` Eric Botcazou
0 siblings, 0 replies; 10+ messages in thread
From: Eric Botcazou @ 2008-09-27 10:52 UTC (permalink / raw)
To: Richard Henderson; +Cc: gcc-patches, bernd.schmidt, aoliva
> So we're going to avoid putting one 4 byte constant into memory
> in exchange for 12 bytes of instructions to put the constant
> onto the stack? That seems a bit silly to me.
For the testcase the stack is not touched in the end at -O because of DSE:
(insn 5 4 6 2 colors.adb:25 (set (reg:SI 195)
(const_int 16908288 [0x1020000])) 219 {*movsi_internal} (nil))
(insn 6 5 7 2 colors.adb:25 (set (reg:SI 194)
(ior:SI (reg:SI 195)
(const_int 772 [0x304]))) 112 {*iorsi3} (expr_list:REG_EQUAL
(const_int 16909060 [0x1020304])
(nil)))
(insn 7 6 8 2 colors.adb:25 (set (mem/c/i:SI (plus:SI (reg/f:SI 78 $frame)
(const_int 16 [0x10])) [0 S4 A32])
(reg:SI 194)) 219 {*movsi_internal} (nil))
is turned into
(insn 5 4 6 2 colors.adb:25 (set (reg:SI 195)
(const_int 16908288 [0x1020000])) 219 {*movsi_internal} (nil))
(insn 6 5 65 2 colors.adb:25 (set (reg:SI 194)
(ior:SI (reg:SI 195)
(const_int 772 [0x304]))) 112 {*iorsi3} (expr_list:REG_EQUAL
(const_int 16909060 [0x1020304])
(nil)))
(insn 65 6 64 2 colors.adb:25 (set (reg:SI 238)
(reg:SI 194)) -1 (nil))
--
Eric Botcazou
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix ICE on user view-conversion (v2)
2008-09-26 21:35 ` Richard Henderson
2008-09-26 21:46 ` Andrew Pinski
2008-09-26 22:02 ` Eric Botcazou
@ 2008-09-28 1:19 ` Richard Sandiford
2008-09-29 17:07 ` Richard Henderson
2 siblings, 1 reply; 10+ messages in thread
From: Richard Sandiford @ 2008-09-28 1:19 UTC (permalink / raw)
To: Richard Henderson; +Cc: Eric Botcazou, gcc-patches, bernd.schmidt, aoliva
This is probably moot given Eric's later replies, but just in case...
Richard Henderson <rth@redhat.com> writes:
> Eric Botcazou wrote:
>> /* As an optimization, reject constants that mips_legitimize_move
>> can expand inline.
>>
>> Suppose we have a multi-instruction sequence that loads constant C
>> into register R. If R does not get allocated a hard register, and
>> R is used in an operand that allows both registers and memory
>> references, reload will consider forcing C into memory and using
>> one of the instruction's memory alternatives. Returning false
>> here will force it to use an input reload instead. */
>> if (GET_CODE (x) == CONST_INT && LEGITIMATE_CONSTANT_P (x))
>> return true;
>
> I think this is a bug in the mips port. And looking closer, in
> the bfin and frv ports as well. Everyone else appears to actually
> identify those constants that *can't* go to memory. In particular,
> I don't see CONST_INT, CONST_DOUBLE, or CONST_VECTOR being rejected.
...are those the only kinds of constant that can appear here?
Going by the kind of wierdness Ada allows, couldn't we get a
symbolic constant as well? It's then more common to reject
certain illegitimate constants in cannot_force_const_mem.
(E.g. on flat targets, where the relocation symbol + addend
must be within the same segment as the symbol.) If so,
I think we'd need something like Eric's patch for those cases,
even if we change the MIPS COSNT_INT one.
In general -- given the flat and TLS restrictions -- I think every
caller to force_const_mem should cope with the NULL possibility.
Richard
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix ICE on user view-conversion (v2)
2008-09-28 1:19 ` Richard Sandiford
@ 2008-09-29 17:07 ` Richard Henderson
2008-09-29 19:18 ` Eric Botcazou
0 siblings, 1 reply; 10+ messages in thread
From: Richard Henderson @ 2008-09-29 17:07 UTC (permalink / raw)
To: Richard Henderson, Eric Botcazou, gcc-patches, bernd.schmidt,
aoliva, rdsandiford
Richard Sandiford wrote:
> Going by the kind of wierdness Ada allows, couldn't we get a
> symbolic constant as well?
...
> In general -- given the flat and TLS restrictions -- I think every
> caller to force_const_mem should cope with the NULL possibility.
A fair comment. I suppose we should accept Eric's patch and
fix the backends separately anyway.
r~
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2008-09-29 18:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-09-26 20:45 [PATCH] Fix ICE on user view-conversion (v2) Eric Botcazou
2008-09-26 21:35 ` Richard Henderson
2008-09-26 21:46 ` Andrew Pinski
2008-09-26 21:52 ` Richard Henderson
2008-09-26 22:02 ` Eric Botcazou
2008-09-26 23:41 ` Richard Henderson
2008-09-27 10:52 ` Eric Botcazou
2008-09-28 1:19 ` Richard Sandiford
2008-09-29 17:07 ` Richard Henderson
2008-09-29 19:18 ` Eric Botcazou
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).