Incomplete instatitiation of virtual registers

Incomplete instatitiation of virtual registers

[Martin Koegler]

I notice, that your last change in function.c forgets virtual
registers in the RTL in some conditions. In older version (the last I used was 20050412),
this has not happend.

In 01.sibling, I have the instruction:
(insn 10 8 12 1 (set (mem/f/i:HI (plus:HI (reg/f:HI 23 virtual-stack-vars)
                (const_int -2 [0xfffffffe])) [0 b+0 S2 A8 00000000])
        (plus:HI (reg/f:HI 23 virtual-stack-vars)
            (const_int -6 [0xfffffffa]))) -1 (nil)
    (nil))

In 03.jump, it is replaced with
(insn 10 8 12 1 (set (mem/f/i:HI (plus:HI (reg/f:HI 23 virtual-stack-vars)
                (const_int -2 [0xfffffffe])) [0 b+0 S2 A8 00000000])
        (plus:HI (reg/f:HI 6 VFP)
            (const_int -6 [0xfffffffa]))) 73 {addhi3} (nil)
    (nil))

The left virtual-stack-vars causes other problems in the reload pass.

I noticed this in my GCC port, I don't know, if such RTL expressions can
occure in offical versions.

A simple workaround is to rerun the instanication, if anything changes:
Index: function.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/function.c,v
retrieving revision 1.616
diff -u -p -r1.616 function.c
--- function.c  30 Apr 2005 03:17:53 -0000      1.616
+++ function.c  1 May 2005 15:11:22 -0000
@@ -1528,6 +1528,8 @@ instantiate_virtual_regs_in_insn (rtx in
       if (recog_memoized (insn) < 0)
        fatal_insn_not_found (insn);
     }
+  if(any_change)
+    instantiate_virtual_regs_in_insn (insn);
 }

 /* Subroutine of instantiate_decls.  Given RTL representing a decl,


mfg Martin Kögler

Re: Incomplete instatitiation of virtual registers

[James E Wilson]

Martin Koegler wrote:
> I notice, that your last change in function.c forgets virtual
> registers in the RTL in some conditions. In older version (the last I used was 20050412),
> this has not happend.

Patches should go to gcc-patches instead of the gcc list.

If you want us to continue accepting patches from you, then you need to 
fill out a copyright assignment form.

Recursively calling instantiate_virtual_regs_in_insn does not look 
right.  We need a better explanation of what is going wrong here.  Since 
we don't have a copy of your port, you will have to explain in detail 
what happens as virtual register instantiation is happening, in order to 
explain how this occurs.  Normally, we would ask for a testcase, but 
that won't work in this case.

It is also possible that this is a bug in your port, if you are 
accidentally emitting the virtual register yourself somehow during or 
after virtual register instantiation.

I know of one PR that has since been filed for a problem with the new 
virtual register instantiation code.  That is PR 21328.  This doesn't 
appear to be related.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: Incomplete instatitiation of virtual registers

[Richard Henderson]

On Tue, May 03, 2005 at 05:44:47PM -0700, James E Wilson wrote:
> Recursively calling instantiate_virtual_regs_in_insn does not look 
> right.

Indeed it is not.

I'd like to see the define_insn for {addhi3}.  I'm a bit confused as
to how I could have missed iterating over what appears like it ought
to be match_operand 0.

> I know of one PR that has since been filed for a problem with the new 
> virtual register instantiation code.  That is PR 21328.

Actually, 21318, but yes this is unrelated.  The symptom in that case
is an ICE in simplify_subreg.



r~

Re: Incomplete instatitiation of virtual registers

[Martin Koegler]

On Tue, May 03, 2005 at 05:44:47PM -0700, James E Wilson wrote:
> Martin Koegler wrote:
> >I notice, that your last change in function.c forgets virtual
> >registers in the RTL in some conditions. In older version (the last I used 
> >was 20050412),
> >this has not happend.
> 
> Patches should go to gcc-patches instead of the gcc list.
> 
> If you want us to continue accepting patches from you, then you need to 
> fill out a copyright assignment form.
> 
> Recursively calling instantiate_virtual_regs_in_insn does not look 
> right.  We need a better explanation of what is going wrong here.  Since 
> we don't have a copy of your port, you will have to explain in detail 
> what happens as virtual register instantiation is happening, in order to 
> explain how this occurs.  Normally, we would ask for a testcase, but 
> that won't work in this case.

In that case, I didn't want to get this patch in GCC. In only wanted to
inform, that the code forgets some registers and that a rerun of 
instantiate_virtual_regs_in_insn solves the problem. I don't know, 
if a bug report is ok in such a case (as I have not reproduced it in
a offical version), so I send a mail to Richard Henderson, who did
the last change with CC to the GCC list.

> It is also possible that this is a bug in your port, if you are 
> accidentally emitting the virtual register yourself somehow during or 
> after virtual register instantiation.

I discovered the problem, while running a regression test, after
I have updated the GCC core. Many compilations failed in
refers_to_regno_for_reload_p. It turned out, that the instantiations
forget some registers. Adding the two lines of code, it worked again.

For that instruction, instantiate_virtual_regs_in_insn
enters if(set), then if (GET_CODE (SET_SRC (set)) == PLUS 
is entered, where if (safe_insn_predicate (insn_code, 1, new) is entered.
It then jumps to verify, without changing the destination, because it is
MEM expression. After the verify label, no more change of the destination will happen.

As far as I have seen, instantiate_virtual_regs_in_insn is called
for each instruction only once.

Richard Henderson wrote:
> I'd like to see the define_insn for {addhi3}.  I'm a bit confused as
> to how I could have missed iterating over what appears like it ought
> to be match_operand 0.

(define_insn_and_split "addhi3"
[(set (match_operand:HI 0 "regmemptne_operand" "=&rATQ,&rRBU,&rRBU")
  (plus:HI
    (match_operand:HI 1 "regmemimmpt_operand" "rATQi,0,rATQI")
    (match_operand:HI 2 "regmemimmst_operand" "rRBUi,rRBUi,0")
))]
....)


I put the patches and new files for the m68hc05 Port again CVS Versions
of GCC, Binutils, GDB and newlib of 20050501 at:
http://www.auto.tuwien.ac.at/~mkoegler/m68hc05/m68hc05.tar.gz (120k)

It is a development shapshot and requires still a lot of work.
I have some not yet published documenations about the internals of the GCC port (predicates, constraints,...),
if it is needed.

Patching and compiling everything, except GCC, is described at
http://www.auto.tuwien.ac.at/~mkoegler/index.php/bcus and nested pages.

For compiling m68hc05-gcc, a patch (gcc.patch) plus the files in gcc/config/m68hc05 are needed.
(In addition to config.sub.patch for adding the processor)

mfg Martin Kögler
e9925248@stud4.tuwien.ac.at

Re: Incomplete instatitiation of virtual registers

[Richard Henderson]

On Wed, May 04, 2005 at 09:50:49AM +0200, Martin Koegler wrote:
> For that instruction, instantiate_virtual_regs_in_insn
> enters if(set), then if (GET_CODE (SET_SRC (set)) == PLUS 
> is entered, where if (safe_insn_predicate (insn_code, 1, new) is entered.
> It then jumps to verify, without changing the destination, because it is
> MEM expression. After the verify label, no more change of the destination
> will happen.

Ah, yes.  Try the following patch.


r~




Index: function.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/function.c,v
retrieving revision 1.620
diff -u -p -r1.620 function.c
--- function.c	4 May 2005 01:38:14 -0000	1.620
+++ function.c	4 May 2005 08:01:11 -0000
@@ -1317,7 +1317,7 @@ instantiate_virtual_regs_in_insn (rtx in
 {
   HOST_WIDE_INT offset;
   int insn_code, i;
-  bool any_change;
+  bool any_change = false;
   rtx set, new, x, seq;
 
   /* There are some special cases to be handled first.  */
@@ -1374,6 +1374,7 @@ instantiate_virtual_regs_in_insn (rtx in
 	}
 
       extract_insn (insn);
+      insn_code = INSN_CODE (insn);
 
       /* Handle a plus involving a virtual register by determining if the
 	 operands remain valid if they're modified in place.  */
@@ -1387,7 +1388,9 @@ instantiate_virtual_regs_in_insn (rtx in
 	  offset += INTVAL (recog_data.operand[2]);
 
 	  /* If the sum is zero, then replace with a plain move.  */
-	  if (offset == 0)
+	  if (offset == 0
+	      && REG_P (SET_DEST (set))
+	      && REGNO (SET_DEST (set)) > LAST_VIRTUAL_REGISTER)
 	    {
 	      start_sequence ();
 	      emit_move_insn (SET_DEST (set), new);
@@ -1400,7 +1403,6 @@ instantiate_virtual_regs_in_insn (rtx in
 	    }
 
 	  x = gen_int_mode (offset, recog_data.operand_mode[2]);
-	  insn_code = INSN_CODE (insn);
 
 	  /* Using validate_change and apply_change_group here leaves
 	     recog_data in an invalid state.  Since we know exactly what
@@ -1411,15 +1413,17 @@ instantiate_virtual_regs_in_insn (rtx in
 	      *recog_data.operand_loc[1] = recog_data.operand[1] = new;
 	      *recog_data.operand_loc[2] = recog_data.operand[2] = x;
 	      any_change = true;
-	      goto verify;
+
+	      /* Fall through into the regular operand fixup loop in
+		 order to take care of operands other than 1 and 2.  */
 	    }
 	}
     }
   else
-    extract_insn (insn);
-
-  insn_code = INSN_CODE (insn);
-  any_change = false;
+    {
+      extract_insn (insn);
+      insn_code = INSN_CODE (insn);
+    }
 
   /* In the general case, we expect virtual registers to appear only in
      operands, and then only as either bare registers or inside memories.  */
@@ -1503,7 +1507,6 @@ instantiate_virtual_regs_in_insn (rtx in
       any_change = true;
     }
 
- verify:
   if (any_change)
     {
       /* Propagate operand changes into the duplicates.  */

Re: Incomplete instatitiation of virtual registers

[Martin Koegler]

On Wed, May 04, 2005 at 01:02:18AM -0700, Richard Henderson wrote:
> On Wed, May 04, 2005 at 09:50:49AM +0200, Martin Koegler wrote:
> > For that instruction, instantiate_virtual_regs_in_insn
> > enters if(set), then if (GET_CODE (SET_SRC (set)) == PLUS 
> > is entered, where if (safe_insn_predicate (insn_code, 1, new) is entered.
> > It then jumps to verify, without changing the destination, because it is
> > MEM expression. After the verify label, no more change of the destination
> > will happen.
> 
> Ah, yes.  Try the following patch.

For the test case, I analyzed, this patch works. I have started a regression test.
Will take some time, before I can tell you the final result.

mfg Martin Kögler

Re: Incomplete instatitiation of virtual registers

[Martin Koegler]

On Wed, May 04, 2005 at 01:02:18AM -0700, Richard Henderson wrote:
> On Wed, May 04, 2005 at 09:50:49AM +0200, Martin Koegler wrote:
> > For that instruction, instantiate_virtual_regs_in_insn
> > enters if(set), then if (GET_CODE (SET_SRC (set)) == PLUS 
> > is entered, where if (safe_insn_predicate (insn_code, 1, new) is entered.
> > It then jumps to verify, without changing the destination, because it is
> > MEM expression. After the verify label, no more change of the destination
> > will happen.
> 
> Ah, yes.  Try the following patch.

Regression test with this patch has finished without any more problem
related to the change in the register instantiation.

From my point of view, this patch can be used as fix for my problem.

mfg Martin Kögler