[PATCH] Fix ICE on user view-conversion (v2)

[PATCH] Fix ICE on user view-conversion (v2)

[Eric Botcazou]

[-- 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);

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Richard Henderson]

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~

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Andrew Pinski]

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

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Richard Henderson]

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~

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Eric Botcazou]

> 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

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Richard Henderson]

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~

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Eric Botcazou]

> 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

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Richard Sandiford]

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

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Richard Henderson]

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~

Re: [PATCH] Fix ICE on user view-conversion (v2)

[Eric Botcazou]

> A fair comment.  I suppose we should accept Eric's patch and
> fix the backends separately anyway.

OK, now installed, thank you (and the other Richard).

-- 
Eric Botcazou