[PATCH, MIPS] Fix build after no_new_pseudos

[PATCH, MIPS] Fix build after no_new_pseudos

[Adam Nemet]

This implements what I mentioned in
http://gcc.gnu.org/ml/gcc/2007-07/msg00528.html.  It turns out that
besides mips_split_symbol another helper mips_move_integer is also
called from a splitter and thus requires the same handling.

My solution is to have the caller specify whether creating pseudos is
permitted.  Callers usually pass what is returned by
can_create_pseudo_p() except splitters that always pass false.

With this change the build completes fine now.  There are still some
regressions from last week (r126353).  We get an ICE on a few tests:

  gcc.c-torture/compile/nested-1.c:9: internal compiler error: in emit_library_call_value_1, at calls.c:3447
  Please submit a full bug report,
  with preprocessed source if appropriate.
  See <URL:http://gcc.gnu.org/bugs.html> for instructions.

Has anybody seen this?  I don't think it is related to no_new_pseudos.
I will look at it later today.

The build and the testing was done on mipsisa64-elf.

OK?

Adam


	* config/mips/mips.c (mips_force_temporary, mips_move_integer):
	Add new argument can_create_pseudo_p and use it instead of calling
	can_create_pseudo_p().
	(mips_split_symbol): Likewise.  Forward can_create_pseudo_p to
	mips_force_temporary.
	(mips_unspec_offset_high, mips_add_offset,
	mips_legitimize_const_move, mips_legitimize_address): Update calls
	to mips_split_symbol, mips_force_temporary, mips_move_integer.
	* config/mips/mips-protos.h (mips_split_symbol,
	mips_move_integer): Update declaration.
	* config/mips/mips.md (splittable_const_int_operand splitter):
	Update call to mips_move_integer.
	(splittable_symbolic_operand splitter): Update call to
	mips_split_symbol.

Index: config/mips/mips.md
===================================================================
--- config/mips/mips.md	(revision 126640)
+++ config/mips/mips.md	(working copy)
@@ -3312,7 +3312,7 @@ (define_split
   ""
   [(const_int 0)]
 {
-  mips_move_integer (operands[0], operands[2], INTVAL (operands[1]));
+  mips_move_integer (operands[0], operands[2], INTVAL (operands[1]), false);
   DONE;
 })
 
@@ -3323,7 +3323,7 @@ (define_split
    (clobber (match_operand:P 2 "register_operand"))]
   ""
   [(set (match_dup 0) (match_dup 1))]
-  { operands[1] = mips_split_symbol (operands[2], operands[1]); })
+  { operands[1] = mips_split_symbol (operands[2], operands[1], false); })
 
 ;; 64-bit integer moves
 
Index: config/mips/mips-protos.h
===================================================================
--- config/mips/mips-protos.h	(revision 126640)
+++ config/mips/mips-protos.h	(working copy)
@@ -148,10 +148,10 @@ extern int mips_idiv_insns (void);
 extern int fp_register_operand (rtx, enum machine_mode);
 extern int lo_operand (rtx, enum machine_mode);
 extern bool mips_legitimate_address_p (enum machine_mode, rtx, int);
-extern rtx mips_split_symbol (rtx, rtx);
+extern rtx mips_split_symbol (rtx, rtx, bool);
 extern rtx mips_unspec_address (rtx, enum mips_symbol_type);
 extern bool mips_legitimize_address (rtx *, enum machine_mode);
-extern void mips_move_integer (rtx, rtx, unsigned HOST_WIDE_INT);
+extern void mips_move_integer (rtx, rtx, unsigned HOST_WIDE_INT, bool);
 extern bool mips_legitimize_move (enum machine_mode, rtx, rtx);
 
 extern int m16_uimm3_b (rtx, enum machine_mode);
Index: config/mips/mips.c
===================================================================
--- config/mips/mips.c	(revision 126640)
+++ config/mips/mips.c	(working copy)
@@ -297,7 +297,7 @@ static bool mips_cannot_force_const_mem 
 static bool mips_use_blocks_for_constant_p (enum machine_mode, rtx);
 static int mips_symbol_insns (enum mips_symbol_type);
 static bool mips16_unextended_reference_p (enum machine_mode mode, rtx, rtx);
-static rtx mips_force_temporary (rtx, rtx);
+static rtx mips_force_temporary (rtx, rtx, bool);
 static rtx mips_unspec_offset_high (rtx, rtx, rtx, enum mips_symbol_type);
 static rtx mips_add_offset (rtx, rtx, HOST_WIDE_INT);
 static unsigned int mips_build_shift (struct mips_integer_op *, HOST_WIDE_INT);
@@ -2092,13 +2092,14 @@ mips_legitimate_address_p (enum machine_
 }
 
 
-/* Copy VALUE to a register and return that register.  If new psuedos
-   are allowed, copy it into a new register, otherwise use DEST.  */
+/* Copy VALUE to a register and return that register.  If
+   CAN_CREATE_PSEUDO_P is true, copy it into a new register, otherwise use
+   DEST.  */
 
 static rtx
-mips_force_temporary (rtx dest, rtx value)
+mips_force_temporary (rtx dest, rtx value, bool can_create_pseudo_p)
 {
-  if (can_create_pseudo_p ())
+  if (can_create_pseudo_p)
     return force_reg (Pmode, value);
   else
     {
@@ -2109,16 +2110,18 @@ mips_force_temporary (rtx dest, rtx valu
 
 
 /* Return a LO_SUM expression for ADDR.  TEMP is as for mips_force_temporary
-   and is used to load the high part into a register.  */
+   and is used to load the high part into a register.  Only create new
+   pseudos if CAN_CREATE_PSEUDO_P is true.  */
 
 rtx
-mips_split_symbol (rtx temp, rtx addr)
+mips_split_symbol (rtx temp, rtx addr, bool can_create_pseudo_p)
 {
   rtx high;
 
   if (!TARGET_MIPS16)
-    high = mips_force_temporary (temp, gen_rtx_HIGH (Pmode, copy_rtx (addr)));
-  else if (!can_create_pseudo_p ())
+    high = mips_force_temporary (temp, gen_rtx_HIGH (Pmode, copy_rtx (addr)),
+				 can_create_pseudo_p);
+  else if (!can_create_pseudo_p)
     {
       emit_insn (gen_load_const_gp (copy_rtx (temp)));
       high = temp;
@@ -2159,8 +2162,9 @@ mips_unspec_offset_high (rtx temp, rtx b
   if (mips_split_p[symbol_type])
     {
       addr = gen_rtx_HIGH (Pmode, mips_unspec_address (addr, symbol_type));
-      addr = mips_force_temporary (temp, addr);
-      return mips_force_temporary (temp, gen_rtx_PLUS (Pmode, addr, base));
+      addr = mips_force_temporary (temp, addr, can_create_pseudo_p ());
+      return mips_force_temporary (temp, gen_rtx_PLUS (Pmode, addr, base),
+				   can_create_pseudo_p ());
     }
   return base;
 }
@@ -2189,8 +2193,9 @@ mips_add_offset (rtx temp, rtx reg, HOST
 	  high = GEN_INT (CONST_HIGH_PART (offset));
 	  offset = CONST_LOW_PART (offset);
 	}
-      high = mips_force_temporary (temp, high);
-      reg = mips_force_temporary (temp, gen_rtx_PLUS (Pmode, high, reg));
+      high = mips_force_temporary (temp, high, can_create_pseudo_p ());
+      reg = mips_force_temporary (temp, gen_rtx_PLUS (Pmode, high, reg),
+				  can_create_pseudo_p ());
     }
   return plus_constant (reg, offset);
 }
@@ -2329,7 +2334,7 @@ mips_legitimize_address (rtx *xloc, enum
       && mips_symbolic_address_p (symbol_type, mode)
       && mips_split_p[symbol_type])
     {
-      *xloc = mips_split_symbol (0, *xloc);
+      *xloc = mips_split_symbol (0, *xloc, can_create_pseudo_p ());
       return true;
     }
 
@@ -2451,10 +2456,12 @@ mips_build_integer (struct mips_integer_
 }
 
 
-/* Load VALUE into DEST, using TEMP as a temporary register if need be.  */
+/* Load VALUE into DEST, using TEMP as a temporary register if need be.
+   Only create new pseudos if CAN_CREATE_PSEUDO_P is true.  */
 
 void
-mips_move_integer (rtx dest, rtx temp, unsigned HOST_WIDE_INT value)
+mips_move_integer (rtx dest, rtx temp, unsigned HOST_WIDE_INT value,
+		   bool can_create_pseudo_p)
 {
   struct mips_integer_op codes[MIPS_MAX_INTEGER_OPS];
   enum machine_mode mode;
@@ -2469,7 +2476,7 @@ mips_move_integer (rtx dest, rtx temp, u
   x = GEN_INT (codes[0].value);
   for (i = 1; i < cost; i++)
     {
-      if (!can_create_pseudo_p ())
+      if (!can_create_pseudo_p)
 	{
 	  emit_insn (gen_rtx_SET (VOIDmode, temp, x));
 	  x = temp;
@@ -2495,14 +2502,16 @@ mips_legitimize_const_move (enum machine
   /* Split moves of big integers into smaller pieces.  */
   if (splittable_const_int_operand (src, mode))
     {
-      mips_move_integer (dest, dest, INTVAL (src));
+      mips_move_integer (dest, dest, INTVAL (src), can_create_pseudo_p ());
       return;
     }
 
   /* Split moves of symbolic constants into high/low pairs.  */
   if (splittable_symbolic_operand (src, mode))
     {
-      emit_insn (gen_rtx_SET (VOIDmode, dest, mips_split_symbol (dest, src)));
+      rtx t = mips_split_symbol (dest, src, can_create_pseudo_p ());
+
+      emit_insn (gen_rtx_SET (VOIDmode, dest, t));
       return;
     }
 
@@ -2520,7 +2529,7 @@ mips_legitimize_const_move (enum machine
       && offset != const0_rtx
       && (can_create_pseudo_p () || SMALL_INT (offset)))
     {
-      base = mips_force_temporary (dest, base);
+      base = mips_force_temporary (dest, base, can_create_pseudo_p ());
       emit_move_insn (dest, mips_add_offset (0, base, INTVAL (offset)));
       return;
     }
@@ -2530,8 +2539,12 @@ mips_legitimize_const_move (enum machine
   /* When using explicit relocs, constant pool references are sometimes
      not legitimate addresses.  */
   if (!memory_operand (src, VOIDmode))
-    src = replace_equiv_address (src, mips_split_symbol (dest, XEXP (src, 0)));
-  emit_move_insn (dest, src);
+    {
+      rtx t = mips_split_symbol (dest, XEXP (src, 0), can_create_pseudo_p ());
+
+      src = replace_equiv_address (src, t);
+      emit_move_insn (dest, src);
+    }
 }
 
 

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Richard Sandiford]

Adam Nemet <anemet@caviumnetworks.com> writes:
> This implements what I mentioned in
> http://gcc.gnu.org/ml/gcc/2007-07/msg00528.html.  It turns out that
> besides mips_split_symbol another helper mips_move_integer is also
> called from a splitter and thus requires the same handling.
>
> My solution is to have the caller specify whether creating pseudos is
> permitted.  Callers usually pass what is returned by
> can_create_pseudo_p() except splitters that always pass false.

I'd like Ian to confirm whether combine splitters are still not allowed
to create new pseudos, or whether this was something that should now be
relaxed.  I got the impression from the thread leading up to this change
that, after df, we're supposed to be able to create pseudo registers
freely before reload.  If this doesn't apply to combine splitters,
perhaps we should go back to setting can_create_new_pseudos_p to false
when calling them.  That seems cleaner than having to pass a
"can_create_pseudos" argument around the backend to simulate the
same behaviour.

> With this change the build completes fine now.  There are still some
> regressions from last week (r126353).  We get an ICE on a few tests:
>
>   gcc.c-torture/compile/nested-1.c:9: internal compiler error: in emit_library_call_value_1, at calls.c:3447
>   Please submit a full bug report,
>   with preprocessed source if appropriate.
>   See <URL:http://gcc.gnu.org/bugs.html> for instructions.
>
> Has anybody seen this?  I don't think it is related to no_new_pseudos.
> I will look at it later today.

This is probably fallout from the __builtin_cache_flush change.  When
reviewing it, I wrongly thought that emit_library_call_value_1 would
convert arguments, so no special conversion code would be needed for
Pmode != SImode.  I'll look into it.

Richard

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Ian Lance Taylor]

Richard Sandiford <richard@codesourcery.com> writes:

> Adam Nemet <anemet@caviumnetworks.com> writes:
> > This implements what I mentioned in
> > http://gcc.gnu.org/ml/gcc/2007-07/msg00528.html.  It turns out that
> > besides mips_split_symbol another helper mips_move_integer is also
> > called from a splitter and thus requires the same handling.
> >
> > My solution is to have the caller specify whether creating pseudos is
> > permitted.  Callers usually pass what is returned by
> > can_create_pseudo_p() except splitters that always pass false.
> 
> I'd like Ian to confirm whether combine splitters are still not allowed
> to create new pseudos, or whether this was something that should now be
> relaxed.  I got the impression from the thread leading up to this change
> that, after df, we're supposed to be able to create pseudo registers
> freely before reload.  If this doesn't apply to combine splitters,
> perhaps we should go back to setting can_create_new_pseudos_p to false
> when calling them.  That seems cleaner than having to pass a
> "can_create_pseudos" argument around the backend to simulate the
> same behaviour.

I think we should fix combine to permit splitters to create new
pseudos.  I don't think it does us any useful service to prohibit
them.

Ian

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Richard Kenner]

> I think we should fix combine to permit splitters to create new
> pseudos.  I don't think it does us any useful service to prohibit
> them.

What if the splitter doesn't result in a recognized insn (most don't).
We could potentially be creating a lot of unneeded pseudos.

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Ian Lance Taylor]

kenner@vlsi1.ultra.nyu.edu (Richard Kenner) writes:

> > I think we should fix combine to permit splitters to create new
> > pseudos.  I don't think it does us any useful service to prohibit
> > them.
> 
> What if the splitter doesn't result in a recognized insn (most don't).
> We could potentially be creating a lot of unneeded pseudos.

I'm not sure I agree.  I suspect that most splitters, when they
succeed, do result in recognized insns.

Conversely, some splitters may require a new pseudo, and yet be quite
useful if they can be run during combine.

In any case, this is something we can measure.

Ian

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Adam Nemet]

Ian Lance Taylor writes:
> I think we should fix combine to permit splitters to create new
> pseudos.  I don't think it does us any useful service to prohibit
> them.

OK so if I understand correctly you're proposing a slight change in
the combine-backend interface.  Besides using the pseudos provided
through a clobber now even new pseudo could be used.  Then combine
would have to scan the insn for those new pseudos and update reg_stat?

Adam

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Richard Sandiford]

Richard Sandiford <richard@codesourcery.com> writes:
> Adam Nemet <anemet@caviumnetworks.com> writes:
>> With this change the build completes fine now.  There are still some
>> regressions from last week (r126353).  We get an ICE on a few tests:
>>
>>   gcc.c-torture/compile/nested-1.c:9: internal compiler error: in emit_library_call_value_1, at calls.c:3447
>>   Please submit a full bug report,
>>   with preprocessed source if appropriate.
>>   See <URL:http://gcc.gnu.org/bugs.html> for instructions.
>>
>> Has anybody seen this?  I don't think it is related to no_new_pseudos.
>> I will look at it later today.
>
> This is probably fallout from the __builtin_cache_flush change.  When
> reviewing it, I wrongly thought that emit_library_call_value_1 would
> convert arguments, so no special conversion code would be needed for
> Pmode != SImode.  I'll look into it.

I've installed the patch below after regression-testing on
mipsisa64-elf.  The choice between Pmode and SImode doesn't matter for
traditional uses of -mflush-func=, since we previously only used the
function for flushing TRAMPOLINE_SIZE bytes.  The flush function really
ought to cope with a Pmode-size (== sizeof (size_t) for sane size_t)
range if __builtin_clear_cache is to work correctly on very large
ranges, but in practice, we're only likely to be flushing <2G anyway.

Richard


gcc/
	* config/mips/mips.md (clear_cache): Treat the size argument as Pmode.

Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md	2007-07-18 10:35:11.000000000 +0100
+++ gcc/config/mips/mips.md	2007-07-18 10:35:45.000000000 +0100
@@ -4249,8 +4249,7 @@ (define_expand "clear_cache"
       /* Flush both caches.  We need to flush the data cache in case
          the system has a write-back cache.  */
       emit_library_call (gen_rtx_SYMBOL_REF (Pmode, mips_cache_flush_func),
-                         0, VOIDmode, 3, operands[0], Pmode,
-                         len, TYPE_MODE (integer_type_node),
+                         0, VOIDmode, 3, operands[0], Pmode, len, Pmode,
                          GEN_INT (3), TYPE_MODE (integer_type_node));
    }
   DONE;

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Sandra Loosemore]

Just checking up on the status of this patch -- are we getting any closer to 
having mainline head be buildable again?

-Sandra

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Adam Nemet]

Sandra Loosemore writes:
> Just checking up on the status of this patch -- are we getting any
> closer to having mainline head be buildable again?

The latest I know about this is that Ian is working on it.  He sent me
a preliminary patch that converted combine's reg_stat array to a VEC
during the summit.  I played around with that patch and changed
try_combine to discover one new additional pseudo in the new insns
generated by the backend.  It appears to me that handling any number
of new pseudos will be more complicated but Ian might have already
figured out a better way of doing this.

Adam

Re: [PATCH, MIPS] Fix build after no_new_pseudos

[Ian Lance Taylor]

Sandra Loosemore <sandra@codesourcery.com> writes:

> Just checking up on the status of this patch -- are we getting any
> closer to having mainline head be buildable again?

This should now be fixed with commit 126942.

Sorry for the delay.

Ian