Patchwork find_movable_pseudos vs. split moves (was Re: rx-elf fails after r187015)

login
register
mail settings
Submitter Richard Sandiford
Date May 15, 2012, 1:08 p.m.
Message ID <g4d365d22n.fsf@richards-thinkpad.stglab.manchester.uk.ibm.com>
Download mbox | patch
Permalink /patch/159317/
State New
Headers show

Comments

Richard Sandiford - May 15, 2012, 1:08 p.m.
DJ Delorie <dj@redhat.com> writes:
> After r187015 (Mar 31), gcc builds for rx-elf are failing with:
>
> make[3]: Entering directory `/greed/dj/m32c/gcc/rx-elf-head/rx-elf/64-bit-double/libgcc'
> # If this is the top-level multilib, build all the other
> # multilibs.
> /greed/dj/m32c/gcc/rx-elf-head/./gcc/xgcc -B/greed/dj/m32c/gcc/rx-elf-head/./gcc/ -B/greed/dj/m32c/install/rx-elf/bin/ -B/greed/dj/m32c/install/rx-elf/lib/ -isystem /greed/dj/m32c/install/rx-elf/include -isystem /greed/dj/m32c/install/rx-elf/sys-include    -g -O2 -m64bit-doubles -O2  -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE  -W -Wall -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition  -isystem ./include   -g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector -Dinhibit_libc  -I. -I. -I../../.././gcc -I../../../../gcc/libgcc -I../../../../gcc/libgcc/. -I../../../../gcc/libgcc/../gcc -I../../../../gcc/libgcc/../include  -DHAVE_CC_TLS -DUSE_EMUTLS -o _mulvdi3.o -MT _mulvdi3.o -MD -MP -MF _mulvdi3.dep -DL_mulvdi3 -c ../../../../gcc/libgcc/libgcc2.c 
> ../../../../gcc/libgcc/libgcc2.c: In function '__mulvdi3':
> ../../../../gcc/libgcc/libgcc2.c:397:1: internal compiler error: in subreg_get_info, at rtlanal.c:3308
>  }
>  ^
>
> Can anyone else confirm this?

I can well believe that that patch exposed the problem, but the bug
seems to be in IRA.  find_moveable_pseudos tries to redirect a DImode
multiplication to a new register, then copies the new register to the
original one before the first use.  The problem here is that rx has no
DImode move pattern, so we end up splitting the DImode move into two
SImode moves and a clobber.  When allocation fails for the new register,
move_unallocated_pseudos deletes one of these three instructions,
but the other two are left around.  reload then gets confused because
we have a subreg of a pseudo that isn't allocated and has no stack slot.

One fix would be to check whether the backend has a move pattern.
I don't particularly like that though, since I don't think targets
like rx should be punished for not defining a DImode move.  The fact
that the first lower-subreg pass is so early in the rtl pipeline
suggests that having unitary multiword moves shouldn't be important
these days.

This patch instead deletes all instructions that set the new register.
One advantage is that IRA no longer needs to track the move specially.

Tested by making sure that rx-elf build again and that the new code
deletes all three instructions generated by gen_move_insn.  Also
bootstrapped & regression-tested on x86_64-linux-gnu.  OK to install?

Richard


gcc/
	* ira.c (pseudo_move_insn): Delete.
	(find_moveable_pseudos): Don't set it.
	(move_unallocated_pseudos): Use DF_REG_DEF_CHAIN to find
	the definitions of the original pseudo.  Delete all of them.
Vladimir Makarov - May 15, 2012, 8:38 p.m.
On 05/15/2012 09:08 AM, Richard Sandiford wrote:
> DJ Delorie<dj@redhat.com>  writes:
>> After r187015 (Mar 31), gcc builds for rx-elf are failing with:
>>
>> make[3]: Entering directory `/greed/dj/m32c/gcc/rx-elf-head/rx-elf/64-bit-double/libgcc'
>> # If this is the top-level multilib, build all the other
>> # multilibs.
>> /greed/dj/m32c/gcc/rx-elf-head/./gcc/xgcc -B/greed/dj/m32c/gcc/rx-elf-head/./gcc/ -B/greed/dj/m32c/install/rx-elf/bin/ -B/greed/dj/m32c/install/rx-elf/lib/ -isystem /greed/dj/m32c/install/rx-elf/include -isystem /greed/dj/m32c/install/rx-elf/sys-include    -g -O2 -m64bit-doubles -O2  -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE  -W -Wall -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition  -isystem ./include   -g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector -Dinhibit_libc  -I. -I. -I../../.././gcc -I../../../../gcc/libgcc -I../../../../gcc/libgcc/. -I../../../../gcc/libgcc/../gcc -I../../../../gcc/libgcc/../include  -DHAVE_CC_TLS -DUSE_EMUTLS -o _mulvdi3.o -MT _mulvdi3.o -MD -MP -MF _mulvdi3.dep -DL_mulvdi3 -c ../../../../gcc/libgcc/libgcc2.c
>> ../../../../gcc/libgcc/libgcc2.c: In function '__mulvdi3':
>> ../../../../gcc/libgcc/libgcc2.c:397:1: internal compiler error: in subreg_get_info, at rtlanal.c:3308
>>   }
>>   ^
>>
>> Can anyone else confirm this?
> I can well believe that that patch exposed the problem, but the bug
> seems to be in IRA.  find_moveable_pseudos tries to redirect a DImode
> multiplication to a new register, then copies the new register to the
> original one before the first use.  The problem here is that rx has no
> DImode move pattern, so we end up splitting the DImode move into two
> SImode moves and a clobber.  When allocation fails for the new register,
> move_unallocated_pseudos deletes one of these three instructions,
> but the other two are left around.  reload then gets confused because
> we have a subreg of a pseudo that isn't allocated and has no stack slot.
>
> One fix would be to check whether the backend has a move pattern.
> I don't particularly like that though, since I don't think targets
> like rx should be punished for not defining a DImode move.  The fact
> that the first lower-subreg pass is so early in the rtl pipeline
> suggests that having unitary multiword moves shouldn't be important
> these days.
>
> This patch instead deletes all instructions that set the new register.
> One advantage is that IRA no longer needs to track the move specially.
>
> Tested by making sure that rx-elf build again and that the new code
> deletes all three instructions generated by gen_move_insn.  Also
> bootstrapped&  regression-tested on x86_64-linux-gnu.  OK to install?
>
> Richard
>
>
> gcc/
> 	* ira.c (pseudo_move_insn): Delete.
> 	(find_moveable_pseudos): Don't set it.
> 	(move_unallocated_pseudos): Use DF_REG_DEF_CHAIN to find
> 	the definitions of the original pseudo.  Delete all of them.
>
Ok.  Thanks for working on this problem, Richard.
DJ Delorie - May 15, 2012, 9:06 p.m.
> gcc/
> 	* ira.c (pseudo_move_insn): Delete.
> 	(find_moveable_pseudos): Don't set it.
> 	(move_unallocated_pseudos): Use DF_REG_DEF_CHAIN to find
> 	the definitions of the original pseudo.  Delete all of them.

I can build with this.  Thanks!

Patch

Index: gcc/ira.c
===================================================================
--- gcc/ira.c	2012-05-15 10:47:19.000000000 +0100
+++ gcc/ira.c	2012-05-15 11:04:50.888126216 +0100
@@ -3621,9 +3621,6 @@  insn_dominated_by_p (rtx i1, rtx i2, int
    first_moveable_pseudo.  */
 /* The original home register.  */
 static VEC (rtx, heap) *pseudo_replaced_reg;
-/* The move instruction we added to move the value to its original home
-   register.  */
-static VEC (rtx, heap) *pseudo_move_insn;
 
 /* Look for instances where we have an instruction that is known to increase
    register pressure, and whose result is not used immediately.  If it is
@@ -3667,9 +3664,7 @@  find_moveable_pseudos (void)
   bitmap_initialize (&interesting, 0);
 
   first_moveable_pseudo = max_regs;
-  VEC_free (rtx, heap, pseudo_move_insn);
   VEC_free (rtx, heap, pseudo_replaced_reg);
-  VEC_safe_grow (rtx, heap, pseudo_move_insn, max_regs);
   VEC_safe_grow (rtx, heap, pseudo_replaced_reg, max_regs);
 
   df_analyze ();
@@ -3964,10 +3959,8 @@  find_moveable_pseudos (void)
 	  if (validate_change (def_insn, DF_REF_LOC (def), newreg, 0))
 	    {
 	      unsigned nregno = REGNO (newreg);
-	      rtx move = emit_insn_before (gen_move_insn (def_reg, newreg),
-					   use_insn);
+	      emit_insn_before (gen_move_insn (def_reg, newreg), use_insn);
 	      nregno -= max_regs;
-	      VEC_replace (rtx, pseudo_move_insn, nregno, move);
 	      VEC_replace (rtx, pseudo_replaced_reg, nregno, def_reg);
 	    }
 	}
@@ -4010,27 +4003,32 @@  move_unallocated_pseudos (void)
   for (i = first_moveable_pseudo; i < last_moveable_pseudo; i++)
     if (reg_renumber[i] < 0)
       {
-	df_ref def = DF_REG_DEF_CHAIN (i);
 	int idx = i - first_moveable_pseudo;
 	rtx other_reg = VEC_index (rtx, pseudo_replaced_reg, idx);
-	rtx def_insn = DF_REF_INSN (def);
-	rtx move_insn = VEC_index (rtx, pseudo_move_insn, idx);
-	rtx set;
+	rtx def_insn = DF_REF_INSN (DF_REG_DEF_CHAIN (i));
+	/* The use must follow all definitions of OTHER_REG, so we can
+	   insert the new definition immediately after any of them.  */
+	df_ref other_def = DF_REG_DEF_CHAIN (REGNO (other_reg));
+	rtx move_insn = DF_REF_INSN (other_def);
 	rtx newinsn = emit_insn_after (PATTERN (def_insn), move_insn);
+	rtx set;
 	int success;
 
 	if (dump_file)
 	  fprintf (dump_file, "moving def of %d (insn %d now) ",
 		   REGNO (other_reg), INSN_UID (def_insn));
 
+	delete_insn (move_insn);
+	while ((other_def = DF_REG_DEF_CHAIN (REGNO (other_reg))))
+	  delete_insn (DF_REF_INSN (other_def));
+	delete_insn (def_insn);
+
 	set = single_set (newinsn);
 	success = validate_change (newinsn, &SET_DEST (set), other_reg, 0);
 	gcc_assert (success);
 	if (dump_file)
 	  fprintf (dump_file, " %d) rather than keep unallocated replacement %d\n",
 		   INSN_UID (newinsn), i);
-	delete_insn (move_insn);
-	delete_insn (def_insn);
 	SET_REG_N_REFS (i, 0);
       }
 }