[01/10] Fix LRA bug

Message ID 37b228579c172c39ce89c75a673326b9185f92bc.1544611347.git.ams@codesourcery.com
State New
Headers show
Series
  • AMD GCN Port v3
Related show

Commit Message

Andrew Stubbs Dec. 12, 2018, 11:52 a.m.
[This is new patch not included in the previously posted patch sets.]

This patch fixes an ICE building libgfortran/random.c.

The problem was an adddi3 instruction that had an eliminable frame pointer.
GCN adddi3 includes a match_scratch, which LRA substitutes with a REG, and
checks if it can be converted back to a scratch afterwards.  In the meantime,
the add was converted to a move, meaning that the instruction pattern
completely changed, thus causing a segfault when the instruction is revisited
in restore_scratches.

2018-12-12  Andrew Stubbs  <ams@codesourcery.com>

	gcc/
	* gcc/lra-int.h (lra_register_new_scratch_op): Add third parameter.
	* gcc/lra-remat.c (update_scratch_ops): Pass icode to
	lra_register_new_scratch_op.
	* gcc/lra.c (struct sloc): Add icode field.
	(lra_register_new_scratch_op): Add icode parameter.
	Use icode to skip insns that have changed beyond recognition.
---
 gcc/lra-int.h   |  2 +-
 gcc/lra-remat.c |  2 +-
 gcc/lra.c       | 12 ++++++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

Comments

Jeff Law Dec. 13, 2018, 11:49 p.m. | #1
On 12/12/18 4:52 AM, Andrew Stubbs wrote:
> 

> [This is new patch not included in the previously posted patch sets.]

> 

> This patch fixes an ICE building libgfortran/random.c.

> 

> The problem was an adddi3 instruction that had an eliminable frame pointer.

> GCN adddi3 includes a match_scratch, which LRA substitutes with a REG, and

> checks if it can be converted back to a scratch afterwards.  In the meantime,

> the add was converted to a move, meaning that the instruction pattern

> completely changed, thus causing a segfault when the instruction is revisited

> in restore_scratches.

I'm torn here.  There's undocumented rules about this kind of stuff,
largely inherited from the days of "reload".  But I don't actually
recall the specifics of any of those rules.

I don't think there's any inherently wrong with what you've done in LRA.
 My worry is there's other places where this kind of changing the
structure of the underlying insn is going to cause you problems if your
patterns aren't following those undocumented rules.




> 

> 2018-12-12  Andrew Stubbs  <ams@codesourcery.com>

> 

> 	gcc/

> 	* gcc/lra-int.h (lra_register_new_scratch_op): Add third parameter.

> 	* gcc/lra-remat.c (update_scratch_ops): Pass icode to

> 	lra_register_new_scratch_op.

> 	* gcc/lra.c (struct sloc): Add icode field.

> 	(lra_register_new_scratch_op): Add icode parameter.

> 	Use icode to skip insns that have changed beyond recognition.

OK.  But be aware we may have to revisit and look more closely what what
you're doing in your port if we stumble over more problems with reload
changing the structure of your insns and causing problems in the process.

jeff
Andrew Stubbs Dec. 14, 2018, 10:04 a.m. | #2
On 13/12/2018 23:49, Jeff Law wrote:
> OK.  But be aware we may have to revisit and look more closely what what

> you're doing in your port if we stumble over more problems with reload

> changing the structure of your insns and causing problems in the process.


Thanks.

What's novel about this, I think, is that we have two candidates for an 
add instruction, and each clobbers a different condition register. I had 
previously handled this by clobbering both, but that was unsatisfactory, 
so I've changed it to use the match_scratch trick I found used in other 
ports.

Using a match_scratch is clearly common practice, but there does appear 
to have been an assumption in LRA that this wouldn't occur within 
patterns that can be transformed by register elimination. Presumably 
those other ports using match_scratch are not using it for some key 
patterns, or do not use register elimination.

Anyway, this patch should not affect any use case that did not already 
have UB, so I'll get it committed shortly.

Andrew
Andrew Stubbs Dec. 14, 2018, 11:52 a.m. | #3
On 14/12/2018 10:04, Andrew Stubbs wrote:
> Anyway, this patch should not affect any use case that did not already 

> have UB, so I'll get it committed shortly.


Now done. Thanks for the review.

Andrew

Patch

diff --git a/gcc/lra-int.h b/gcc/lra-int.h
index 5267b53..d5ff652 100644
--- a/gcc/lra-int.h
+++ b/gcc/lra-int.h
@@ -335,7 +335,7 @@  extern void lra_create_copy (int, int, int);
 extern lra_copy_t lra_get_copy (int);
 extern bool lra_former_scratch_p (int);
 extern bool lra_former_scratch_operand_p (rtx_insn *, int);
-extern void lra_register_new_scratch_op (rtx_insn *, int);
+extern void lra_register_new_scratch_op (rtx_insn *, int, int);
 
 extern int lra_new_regno_start;
 extern int lra_constraint_new_regno_start;
diff --git a/gcc/lra-remat.c b/gcc/lra-remat.c
index faf74ca..627a248 100644
--- a/gcc/lra-remat.c
+++ b/gcc/lra-remat.c
@@ -1042,7 +1042,7 @@  update_scratch_ops (rtx_insn *remat_insn)
 	    fprintf (lra_dump_file, "	 Assigning the same %d to r%d\n",
 		     REGNO (*loc), hard_regno);
 	}
-      lra_register_new_scratch_op (remat_insn, i);
+      lra_register_new_scratch_op (remat_insn, i, id->icode);
     }
   
 }
diff --git a/gcc/lra.c b/gcc/lra.c
index 5d58d90..14974ed 100644
--- a/gcc/lra.c
+++ b/gcc/lra.c
@@ -2026,6 +2026,7 @@  struct sloc
 {
   rtx_insn *insn; /* Insn where the scratch was.  */
   int nop;  /* Number of the operand which was a scratch.  */
+  int icode;  /* Original icode from which scratch was removed.  */
 };
 
 typedef struct sloc *sloc_t;
@@ -2057,7 +2058,7 @@  lra_former_scratch_operand_p (rtx_insn *insn, int nop)
 /* Register operand NOP in INSN as a former scratch.  It will be
    changed to scratch back, if it is necessary, at the LRA end.  */
 void
-lra_register_new_scratch_op (rtx_insn *insn, int nop)
+lra_register_new_scratch_op (rtx_insn *insn, int nop, int icode)
 {
   lra_insn_recog_data_t id = lra_get_insn_recog_data (insn);
   rtx op = *id->operand_loc[nop];
@@ -2065,6 +2066,7 @@  lra_register_new_scratch_op (rtx_insn *insn, int nop)
   lra_assert (REG_P (op));
   loc->insn = insn;
   loc->nop = nop;
+  loc->icode = icode;
   scratches.safe_push (loc);
   bitmap_set_bit (&scratch_bitmap, REGNO (op));
   bitmap_set_bit (&scratch_operand_bitmap,
@@ -2102,7 +2104,7 @@  remove_scratches (void)
 	      *id->operand_loc[i] = reg
 		= lra_create_new_reg (static_id->operand[i].mode,
 				      *id->operand_loc[i], ALL_REGS, NULL);
-	      lra_register_new_scratch_op (insn, i);
+	      lra_register_new_scratch_op (insn, i, id->icode);
 	      if (lra_dump_file != NULL)
 		fprintf (lra_dump_file,
 			 "Removing SCRATCH in insn #%u (nop %d)\n",
@@ -2136,6 +2138,12 @@  restore_scratches (void)
 	  last = loc->insn;
 	  id = lra_get_insn_recog_data (last);
 	}
+      if (loc->icode != id->icode)
+	{
+	  /* The icode doesn't match, which means the insn has been modified
+	     (e.g. register elimination).  The scratch cannot be restored.  */
+	  continue;
+	}
       if (REG_P (*id->operand_loc[loc->nop])
 	  && ((regno = REGNO (*id->operand_loc[loc->nop]))
 	      >= FIRST_PSEUDO_REGISTER)