gcse fix

gcse fix

[Ulf Carlsson]

Hi,

There was a problem with Linux kernel compiles when we used GCC 2.96,
and I have a patch that solves this problem.  The fix is rather
obvious - we verify that the copy register instruction we generate
when optimize really is valid.

I sent the bug report[1] to the gcc-bugs last week.  Keith Wesolowski
has also written a detailed description of the problem:

[Beginning of description by Keith M Wesolowski]

The function of GCSE is to eliminate pieces of code that end up
looking the same. So for example,

	x = y + 4;
	z = y + 4;

ends up looking like
	
	z = y + 4;
	x = z;

Provided that x is not changed between the original two statements, of
course. And this is key: if the compiler thinks the expression hasn't
changed, it will be removed.

In our case, we have

	__asm__("":"=r" (foo)); __asm__("":"=r" (bar));

The rtl corresponding to the two asm sets are:

(insn 14 27 16 (set (reg:QI 83)
        (asm_operands ("") ("=r") 0[ ]
            [ ]  ("foo.c") 7)) -1 (nil)
    (nil))

and

(insn 19 18 21 (set (reg/v:SI 82)
        (asm_operands ("") ("=r") 0[ ]
            [ ]  ("foo.c") 7)) -1 (nil)
    (nil))

Note that both include a 7 in the instruction description - this is
the line number of the inline asm. Since they're on the same line, and
contain the same code and arguments, *** they are the same
instruction. *** The compiler is not smart enough to know that the
instruction operates on operands of two different modes and thus
should be considered different - all it knows is the text inside
__asm__() and the line number.

So the pre_gcse pass removes the first copy of the __asm__ and copies
the value of the output of the last instance of it (word) into the
output register for the first instance of it (byte). Copying an SI (32
bits) into a QI (8 bits) requires a subreg, but the compiler doesn't
generate one. ICE.

Consistent with my explanation, any of the following eliminates the ICE:

	- using __asm__ volatile (...) instead of __asm__ (...)
	- switching the order of dirty (byte) and dirty (word)
	- making sure only one instance of dirty() is ever on a line
	- compiling with -fno-gcse

This is not to say there is no bug; it seems to be that if

	dirty (word); dirty (byte);

is valid and generates valid (even if incorrect - the flip side of
subreg is zero/sign extension) code, then placing them in the opposite
order should do so as well. This requires that gcc be taught to check
the mode of operands in common subexpressions and either a) recognize
them as different if the operands' modes differ, or b) use subreg or
zero/sign extension as appropriate, or c) distinguish among instances
of inline asm even if the instances occur on the same line.

[End of description by Keith M Wesolowski]

We have verified that the patch doesn't break things by boot strapping
the compiler itself on Sparc.  We have also built functional kernels
and glibc with this patch.

Ulf

[1] http://gcc.gnu.org/ml/gcc-bugs/2000-06/msg00646.html

Index: gcse.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/gcse.c,v
retrieving revision 1.89
diff -p -r1.89 gcse.c
*** gcse.c	2000/06/19 01:40:32	1.89
--- gcse.c	2000/07/03 08:09:18
*************** static struct obstack gcse_obstack;
*** 302,308 ****
  /* Non-zero for each mode that supports (set (reg) (reg)).
     This is trivially true for integer and floating point values.
     It may or may not be true for condition codes.  */
! static char can_copy_p[(int) NUM_MACHINE_MODES];
  
  /* Non-zero if can_copy_p has been initialized.  */
  static int can_copy_init_p;
--- 302,308 ----
  /* Non-zero for each mode that supports (set (reg) (reg)).
     This is trivially true for integer and floating point values.
     It may or may not be true for condition codes.  */
! static char can_copy_p[(int) NUM_MACHINE_MODES][(int) NUM_MACHINE_MODES];
  
  /* Non-zero if can_copy_p has been initialized.  */
  static int can_copy_init_p;
*************** gcse_main (f, file)
*** 810,838 ****
  static void
  compute_can_copy ()
  {
!   int i;
! #ifndef AVOID_CCMODE_COPIES
!   rtx reg,insn;
! #endif
    char *free_point = (char *) oballoc (1);
  
!   bzero (can_copy_p, NUM_MACHINE_MODES);
  
    start_sequence ();
    for (i = 0; i < NUM_MACHINE_MODES; i++)
!     if (GET_MODE_CLASS (i) == MODE_CC)
!       {
  #ifdef AVOID_CCMODE_COPIES
! 	can_copy_p[i] = 0;
! #else
! 	reg = gen_rtx_REG ((enum machine_mode) i, LAST_VIRTUAL_REGISTER + 1);
! 	insn = emit_insn (gen_rtx_SET (VOIDmode, reg, reg));
! 	if (recog (PATTERN (insn), insn, NULL_PTR) >= 0)
! 	  can_copy_p[i] = 1;
  #endif
!       }
!     else
!       can_copy_p[i] = 1;
  
    end_sequence ();
  
--- 810,838 ----
  static void
  compute_can_copy ()
  {
!   int i, j;
!   rtx reg1,reg2,insn;
    char *free_point = (char *) oballoc (1);
  
!   bzero (can_copy_p, NUM_MACHINE_MODES * NUM_MACHINE_MODES);
  
    start_sequence ();
    for (i = 0; i < NUM_MACHINE_MODES; i++)
!     for (j = 0; j < NUM_MACHINE_MODES; j++)
  #ifdef AVOID_CCMODE_COPIES
!       if (GET_MODE_CLASS (i) == MODE_CC)
! 	can_copy_p[i][j] = 0;
!       else
  #endif
! 	{
! 	  reg1 = gen_rtx_REG ((enum machine_mode) i,
! 			      LAST_VIRTUAL_REGISTER + 1);
! 	  reg2 = gen_rtx_REG ((enum machine_mode) j,
! 			      LAST_VIRTUAL_REGISTER + 2);
! 	  insn = emit_insn (gen_rtx_SET (VOIDmode, reg1, reg2));
! 	  if (recog (PATTERN (insn), insn, NULL_PTR) >= 0)
! 	    can_copy_p[i][j] = 1;
! 	}
  
    end_sequence ();
  
*************** hash_scan_set (pat, insn, set_p)
*** 1875,1881 ****
        if (! set_p
  	  && regno >= FIRST_PSEUDO_REGISTER
  	  /* Don't GCSE something if we can't do a reg/reg copy.  */
! 	  && can_copy_p [GET_MODE (dest)]
  	  /* Is SET_SRC something we want to gcse?  */
  	  && want_to_gcse_p (src))
  	{
--- 1875,1881 ----
        if (! set_p
  	  && regno >= FIRST_PSEUDO_REGISTER
  	  /* Don't GCSE something if we can't do a reg/reg copy.  */
! 	  && can_copy_p [GET_MODE (dest)][GET_MODE (src)]
  	  /* Is SET_SRC something we want to gcse?  */
  	  && want_to_gcse_p (src))
  	{
*************** hash_scan_set (pat, insn, set_p)
*** 1894,1900 ****
  	       && regno >= FIRST_PSEUDO_REGISTER
  	       && ((GET_CODE (src) == REG
  		    && REGNO (src) >= FIRST_PSEUDO_REGISTER
! 		    && can_copy_p [GET_MODE (dest)])
  		   || GET_CODE (src) == CONST_INT
  		   || GET_CODE (src) == SYMBOL_REF
  		   || GET_CODE (src) == CONST_DOUBLE)
--- 1894,1900 ----
  	       && regno >= FIRST_PSEUDO_REGISTER
  	       && ((GET_CODE (src) == REG
  		    && REGNO (src) >= FIRST_PSEUDO_REGISTER
! 		    && can_copy_p [GET_MODE (dest)][GET_MODE (src)])
  		   || GET_CODE (src) == CONST_INT
  		   || GET_CODE (src) == SYMBOL_REF
  		   || GET_CODE (src) == CONST_DOUBLE)

Re: gcse fix

[Jeffrey A Law]

  In message < 14688.19044.94355.794985@calypso.engr.sgi.com >you write:
   > Note that both include a 7 in the instruction description - this is
  > the line number of the inline asm. Since they're on the same line, and
  > contain the same code and arguments, *** they are the same
  > instruction. *** The compiler is not smart enough to know that the
  > instruction operates on operands of two different modes and thus
  > should be considered different - all it knows is the text inside
  > __asm__() and the line number.
A valid copy can never have different modes for the source and destination
operands, so I would think the changes to can_copy_p and its related 
functions are unwanted/unnecessary.


 > *************** hash_scan_set (pat, insn, set_p)
  > *** 1875,1881 ****
  >         if (! set_p
  >   	  && regno >= FIRST_PSEUDO_REGISTER
  >   	  /* Don't GCSE something if we can't do a reg/reg copy.  */
  > ! 	  && can_copy_p [GET_MODE (dest)]
  >   	  /* Is SET_SRC something we want to gcse?  */
  >   	  && want_to_gcse_p (src))
  >   	{
  > --- 1875,1881 ----
  >         if (! set_p
  >   	  && regno >= FIRST_PSEUDO_REGISTER
  >   	  /* Don't GCSE something if we can't do a reg/reg copy.  */
  > ! 	  && can_copy_p [GET_MODE (dest)][GET_MODE (src)]
  >   	  /* Is SET_SRC something we want to gcse?  */
  >   	  && want_to_gcse_p (src))
  >   	{
Instead I think you just want to say
	&& can_copy_p [GET_MODE (dest)]
	&& GET_MODE (dest) == GET_MODE (src))

Similarly in hash_scan_set.

jeff

Re: gcse fix

[Andreas Jaeger]

>>>>> Jeffrey A Law writes:

Jeff>   In message <14688.19044.94355.794985@calypso.engr.sgi.com>you write:
>> Note that both include a 7 in the instruction description - this is
>> the line number of the inline asm. Since they're on the same line, and
>> contain the same code and arguments, *** they are the same
>> instruction. *** The compiler is not smart enough to know that the
>> instruction operates on operands of two different modes and thus
>> should be considered different - all it knows is the text inside
>> __asm__() and the line number.
Jeff> A valid copy can never have different modes for the source and destination
Jeff> operands, so I would think the changes to can_copy_p and its related 
Jeff> functions are unwanted/unnecessary.


>> *************** hash_scan_set (pat, insn, set_p)
>> *** 1875,1881 ****
>> if (! set_p
>> && regno >= FIRST_PSEUDO_REGISTER
>> /* Don't GCSE something if we can't do a reg/reg copy.  */
>> ! 	  && can_copy_p [GET_MODE (dest)]
>> /* Is SET_SRC something we want to gcse?  */
>> && want_to_gcse_p (src))
>> {
>> --- 1875,1881 ----
>> if (! set_p
>> && regno >= FIRST_PSEUDO_REGISTER
>> /* Don't GCSE something if we can't do a reg/reg copy.  */
>> ! 	  && can_copy_p [GET_MODE (dest)][GET_MODE (src)]
>> /* Is SET_SRC something we want to gcse?  */
>> && want_to_gcse_p (src))
>> {
Jeff> Instead I think you just want to say
Jeff> 	&& can_copy_p [GET_MODE (dest)]
Jeff> 	&& GET_MODE (dest) == GET_MODE (src))

Jeff> Similarly in hash_scan_set.

What is with this patch?  It is still needed for mips and has not been
added yet :-(.

I've just run a make bootstrap on i686-linux-gnu.  Is it ok to commit
this?  I'm giving Jeff credits, hope that's ok.

Andreas

2001-01-14  Jeffrey A Law <law@cygnus.com>

	* gcse.c (hash_scan_set): Don't copy between modes

============================================================
Index: gcc/gcse.c
--- gcc/gcse.c	2001/01/02 00:58:27	1.113
+++ gcc/gcse.c	2001/01/14 10:53:17
@@ -1925,7 +1925,9 @@
 	  /* Don't GCSE something if we can't do a reg/reg copy.  */
 	  && can_copy_p [GET_MODE (dest)]
 	  /* Is SET_SRC something we want to gcse?  */
-	  && want_to_gcse_p (src))
+	  && want_to_gcse_p (src)
+	  /* Copy between modes is prohibited.  */
+	  && GET_MODE (dest) == GET_MODE (src))
 	{
 	  /* An expression is not anticipatable if its operands are
 	     modified before this insn.  */


-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.inka.de
    http://www.suse.de/~aj

Re: gcse fix

[Maciej W. Rozycki]

On 14 Jan 2001, Andreas Jaeger wrote:

> I've just run a make bootstrap on i686-linux-gnu.  Is it ok to commit
> this?  I'm giving Jeff credits, hope that's ok.
> 
> Andreas
> 
> 2001-01-14  Jeffrey A Law <law@cygnus.com>
> 
> 	* gcse.c (hash_scan_set): Don't copy between modes
> 
> ============================================================
> Index: gcc/gcse.c
> --- gcc/gcse.c	2001/01/02 00:58:27	1.113
> +++ gcc/gcse.c	2001/01/14 10:53:17
> @@ -1925,7 +1925,9 @@
>  	  /* Don't GCSE something if we can't do a reg/reg copy.  */
>  	  && can_copy_p [GET_MODE (dest)]
>  	  /* Is SET_SRC something we want to gcse?  */
> -	  && want_to_gcse_p (src))
> +	  && want_to_gcse_p (src)
> +	  /* Copy between modes is prohibited.  */
> +	  && GET_MODE (dest) == GET_MODE (src))
>  	{
>  	  /* An expression is not anticipatable if its operands are
>  	     modified before this insn.  */

 I believe Keith is the author.  The original mail from Ralf follows.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Message-ID: <20000908205810.A11920@bacchus.dhis.org>
Date: Fri, 8 Sep 2000 20:58:11 +0200
From: Ralf Baechle <ralf@oss.sgi.com>
To: Ulf Carlsson <ulfc@engr.sgi.com>,
    Keith M Wesolowski <wesolows@foobazco.org>,
    "Maciej W. Rozycki" <macro@ds2.pg.gda.pl>, Andreas Jaeger <aj@suse.de>
Cc: linux-mips@oss.sgi.com, linux-mips@fnet.fr
Subject: One more gcc patch

Ooops, this fixes a bug in the previous patch for gcc-current.  So this
patch does:

 - fix constructors which were not run for shared libs
 - fix warnings when building the compiler itself
 - Keith's gcse patch
 - gcc was generating code which was calling __main from the beginning of
   main which is wrong for Linux

  Ralf

diff -urN gcc-cygnus/gcc/config/mips/linux.h gcc/gcc/config/mips/linux.h
--- gcc-cygnus/gcc/config/mips/linux.h	Tue Aug 29 02:46:28 2000
+++ gcc/gcc/config/mips/linux.h	Sat Sep  9 17:06:28 2000
@@ -170,3 +170,20 @@
 %{mabi=64: -64} \
 %{!fno-PIC:%{!fno-pic:-KPIC}} \
 %{fno-PIC:-non_shared} %{fno-pic:-non_shared}"
+
+/* On svr4, we *do* have support for the .init and .fini sections, and we
+   can put stuff in there to be executed before and after `main'.  We let
+   crtstuff.c and other files know this by defining the following symbols.
+   The definitions say how to change sections to the .init and .fini
+   sections.  This is the same for all known svr4 assemblers.  */
+
+#define INIT_SECTION_ASM_OP     "\t.section\t.init"
+#define FINI_SECTION_ASM_OP     "\t.section\t.fini"
+
+/* Undef junk imported from mips/elf.h.  */
+#undef CTOR_LIST_BEGIN
+#undef CTOR_LIST_END
+#undef DTOR_LIST_BEGIN
+#undef DTOR_LIST_END
+
+#undef INVOKE__main
diff -urN gcc-cygnus/gcc/config/mips/mips.h gcc/gcc/config/mips/mips.h
--- gcc-cygnus/gcc/config/mips/mips.h	Tue Aug 29 02:46:28 2000
+++ gcc/gcc/config/mips/mips.h	Sat Sep  9 16:07:28 2000
@@ -1900,7 +1900,7 @@
 
 extern enum reg_class mips_char_to_class[];
 
-#define REG_CLASS_FROM_LETTER(C) mips_char_to_class[ (C) ]
+#define REG_CLASS_FROM_LETTER(C) mips_char_to_class[ (int) (C) ]
 
 /* The letters I, J, K, L, M, N, O, and P in a register constraint
    string can be used to stand for particular ranges of immediate
diff -urN gcc-cygnus/gcc/gcse.c gcc/gcc/gcse.c
--- gcc-cygnus/gcc/gcse.c	Mon Sep  4 03:00:56 2000
+++ gcc/gcc/gcse.c	Fri Sep  8 12:37:18 2000
@@ -1924,7 +1924,9 @@
 	  /* Don't GCSE something if we can't do a reg/reg copy.  */
 	  && can_copy_p [GET_MODE (dest)]
 	  /* Is SET_SRC something we want to gcse?  */
-	  && want_to_gcse_p (src))
+	  && want_to_gcse_p (src)
+	  /* Copy between modes is prohibited */
+	  && GET_MODE (src) == GET_MODE (dest))
 	{
 	  /* An expression is not anticipatable if its operands are
 	     modified before this insn.  */

gcse fix

[Jan Hubicka]

Hi,
This bug hit me second time.  The problem is that when we create
REG_EQUAL note for insn in the fialed constant propagation, we
do not copy the rtx properly getting rtl sharing problems that can
cause mysterious crashes later.

I;'ve installed it to the branch. OK for mainline?

Fri Nov 16 19:12:08 CET 2001  Jan Hubicka  <jh@suse.cz>

	* gcse.c (try_replace_reg): Copy RTX before creating note.

*** gcse.c.old	Fri Nov 16 16:01:28 2001
--- gcse.c	Fri Nov 16 17:36:26 2001
*************** try_replace_reg (from, to, insn)
*** 3927,3936 ****
      }
  
    /* If we've failed to do replacement, have a single SET, and don't already
!      have a note, add a REG_EQUAL note to not lose information.  */
!   if (!success && note == 0 && set != 0)
!     note = set_unique_reg_note (insn, REG_EQUAL, src);
  
    /* If there is already a NOTE, update the expression in it with our
       replacement.  */
    else if (note != 0)
--- 3927,3938 ----
      }
  
    /* If we've failed to do replacement, have a single SET, and don't already
!      have a note, add a REG_EQUAL note to not lose information.
  
+      The copy is required, as simplify_replace_rtx does not copy the
+      parts that has not been touched.  */
+   if (!success && note == 0 && set != 0)
+     note = set_unique_reg_note (insn, REG_EQUAL, copy_rtx (src));
    /* If there is already a NOTE, update the expression in it with our
       replacement.  */
    else if (note != 0)

Re: gcse fix

[Richard Henderson]

On Fri, Nov 16, 2001 at 09:13:23PM +0100, Jan Hubicka wrote:
> 	* gcse.c (try_replace_reg): Copy RTX before creating note.

Ok.


r~

gcse fix

[Jeffrey A Law]

I've installed this change.  Things like the frame pointer are not clobberd
by a function call :-)


	* gcse.c (compute_hash_table): Correctly identify hard regs which are
	clobbered across calls.
	(compute_rd_kill): Likewise.

Index: gcse.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/./gcc/gcse.c,v
retrieving revision 1.18
diff -c -3 -p -r1.18 gcse.c
*** gcse.c	1998/10/17 12:11:06	1.18
--- gcse.c	1998/10/20 06:15:40
*************** compute_hash_table (f, set_p)
*** 2075,2081 ****
  	  if (GET_CODE (insn) == CALL_INSN)
  	    {
  	      for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
! 		if (call_used_regs[regno])
  		  record_last_reg_set_info (insn, regno);
  	      if (! CONST_CALL_P (insn))
  		record_last_mem_set_info (insn);
--- 2075,2094 ----
  	  if (GET_CODE (insn) == CALL_INSN)
  	    {
  	      for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
! 		if ((call_used_regs[regno]
! 		     && regno != STACK_POINTER_REGNUM
! #if HARD_FRAME_POINTER_REGNUM != FRAME_POINTER_REGNUM
! 		     && regno != HARD_FRAME_POINTER_REGNUM
! #endif
! #if ARG_POINTER_REGNUM != FRAME_POINTER_REGNUM
! 		     && ! (regno == ARG_POINTER_REGNUM && fixed_regs[regno])
! #endif
! #if defined (PIC_OFFSET_TABLE_REGNUM) && !defined (PIC_OFFSET_TABLE_REG_CALL_CLOBBERED)
! 		     && ! (regno == PIC_OFFSET_TABLE_REGNUM && flag_pic)
! #endif
! 
! 		     && regno != FRAME_POINTER_REGNUM)
! 		    || global_regs[regno])
  		  record_last_reg_set_info (insn, regno);
  	      if (! CONST_CALL_P (insn))
  		record_last_mem_set_info (insn);
*************** compute_kill_rd ()
*** 2548,2554 ****
  
  		  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
                      {
! 		      if (call_used_regs[regno])
  			handle_rd_kill_set (insn, regno, bb);
                      }
                  }
--- 2561,2580 ----
  
  		  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
                      {
! 		      if ((call_used_regs[regno]
! 			   && regno != STACK_POINTER_REGNUM
! #if HARD_FRAME_POINTER_REGNUM != FRAME_POINTER_REGNUM
! 			   && regno != HARD_FRAME_POINTER_REGNUM
! #endif
! #if ARG_POINTER_REGNUM != FRAME_POINTER_REGNUM
! 			   && ! (regno == ARG_POINTER_REGNUM
! 				 && fixed_regs[regno])
! #endif
! #if defined (PIC_OFFSET_TABLE_REGNUM) && !defined (PIC_OFFSET_TABLE_REG_CALL_CLOBBERED)
! 			   && ! (regno == PIC_OFFSET_TABLE_REGNUM && flag_pic)
! #endif
! 			   && regno != FRAME_POINTER_REGNUM)
! 			  || global_regs[regno])
  			handle_rd_kill_set (insn, regno, bb);
                      }
                  }