diff mbox series

combine, v2: Don't record for UNDO_MODE pointers into regno_reg_rtx array [PR104985]

Message ID Yk1UYO1L3l3NUE/F@tucnak
State New
Headers show
Series combine, v2: Don't record for UNDO_MODE pointers into regno_reg_rtx array [PR104985] | expand

Commit Message

Jakub Jelinek April 6, 2022, 8:50 a.m. UTC 
On Tue, Apr 05, 2022 at 04:56:55PM -0500, Segher Boessenkool wrote:
> > Or there are variant patches in the PR, either a minimal version of
> > this patch, or one that records regno and adjusts all SUBST_MODE uses.
> 
> I like this one best I think :-)
> 
> > Also, not sure I like very much a function name in all caps, but I wanted
> > to preserve the users and all other SUBST and SUBST_* are also all caps.
> > Yet another possibility would be keep do_SUBST_MODE with the new
> > arguments and
> > #define SUBST_MODE(INTO, NEWVAL)  do_SUBST_MODE ((INTO), (NEWVAL))
> 
> Like the other things in combine.c do, so please change to that?  Which
> means changing less than you do now :-)
> 
> Changing this all to not have macros is better of course, and can be
> tastefully done even with C++ (it would use C++ features even :-) ), but
> let's do that all at once, and in stage 1.
> 
> > -  union { rtx *r; int *i; struct insn_link **l; } where;
> > +  union { rtx *r; int *i; rtx m; struct insn_link **l; } where;
> 
> NAK.  It is not clear at all what "rtx m" means, esp. since there is an
> "rtx *r" already.  In the PR you said "machine_mode m", that is clear of
> course, can you do that instead?

So in that case something like this (i.e. the regno variant, renamed
to subst_mode from SUBST_MODE, and naming the union member regno rather
than m)?

And for GCC 13 change do_SUBST_INT to subst_int with int &into arg,
do_SUBST_LINK to subst_link with struct insn_link *&into arg,
and do_SUBST into subst with rtx &into arg?

2022-04-06  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/104985
	* combine.cc (struct undo): Add where.regno member.
	(do_SUBST_MODE): Rename to ...
	(subst_mode): ... this.  Change first argument from rtx * into int,
	operate on regno_reg_rtx[regno] and save regno into where.regno.
	(SUBST_MODE): Remove.
	(try_combine): Use subst_mode instead of SUBST_MODE, change first
	argument from regno_reg_rtx[whatever] to whatever.  For UNDO_MODE, use
	regno_reg_rtx[undo->where.regno] instead of *undo->where.r.
	(undo_to_marker): For UNDO_MODE, use regno_reg_rtx[undo->where.regno]
	instead of *undo->where.r.
	(simplify_set): Use subst_mode instead of SUBST_MODE, change first
	argument from regno_reg_rtx[whatever] to whatever.



	Jakub

Comments

Segher Boessenkool April 6, 2022, 4:31 p.m. UTC | #1 
Hi!

So, the core of this problem is once again that regno_reg_rtx is
reallocated.  It will be another decade until we got rid of all fallout
of breaking that guarantee :-(

On Wed, Apr 06, 2022 at 10:50:40AM +0200, Jakub Jelinek wrote:
> On Tue, Apr 05, 2022 at 04:56:55PM -0500, Segher Boessenkool wrote:
> > > -  union { rtx *r; int *i; struct insn_link **l; } where;
> > > +  union { rtx *r; int *i; rtx m; struct insn_link **l; } where;
> > 
> > NAK.  It is not clear at all what "rtx m" means, esp. since there is an
> > "rtx *r" already.  In the PR you said "machine_mode m", that is clear of
> > course, can you do that instead?
> 
> So in that case something like this (i.e. the regno variant, renamed
> to subst_mode from SUBST_MODE, and naming the union member regno rather
> than m)?

I can't say I like that either, the undo for a mode change should just
store the old mode directly, anything else is too fragile.

But, whatever, we'll fix that later.  The patch is okay for trunk.
Thanks!


Segher
diff mbox series

Patch

--- gcc/combine.cc.jj	2022-03-30 09:11:42.331261823 +0200
+++ gcc/combine.cc	2022-04-06 10:29:55.333106447 +0200
@@ -382,7 +382,7 @@  struct undo
   struct undo *next;
   enum undo_kind kind;
   union { rtx r; int i; machine_mode m; struct insn_link *l; } old_contents;
-  union { rtx *r; int *i; struct insn_link **l; } where;
+  union { rtx *r; int *i; int regno; struct insn_link **l; } where;
 };
 
 /* Record a bunch of changes to be undone, up to MAX_UNDO of them.
@@ -761,10 +761,11 @@  do_SUBST_INT (int *into, int newval)
    well.  */
 
 static void
-do_SUBST_MODE (rtx *into, machine_mode newval)
+subst_mode (int regno, machine_mode newval)
 {
   struct undo *buf;
-  machine_mode oldval = GET_MODE (*into);
+  rtx reg = regno_reg_rtx[regno];
+  machine_mode oldval = GET_MODE (reg);
 
   if (oldval == newval)
     return;
@@ -775,15 +776,13 @@  do_SUBST_MODE (rtx *into, machine_mode n
     buf = XNEW (struct undo);
 
   buf->kind = UNDO_MODE;
-  buf->where.r = into;
+  buf->where.regno = regno;
   buf->old_contents.m = oldval;
-  adjust_reg_mode (*into, newval);
+  adjust_reg_mode (reg, newval);
 
   buf->next = undobuf.undos, undobuf.undos = buf;
 }
 
-#define SUBST_MODE(INTO, NEWVAL)  do_SUBST_MODE (&(INTO), (NEWVAL))
-
 /* Similar to SUBST, but NEWVAL is a LOG_LINKS expression.  */
 
 static void
@@ -3186,7 +3185,7 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 		    newpat_dest = gen_rtx_REG (compare_mode, regno);
 		  else
 		    {
-		      SUBST_MODE (regno_reg_rtx[regno], compare_mode);
+		      subst_mode (regno, compare_mode);
 		      newpat_dest = regno_reg_rtx[regno];
 		    }
 		}
@@ -3576,7 +3575,7 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 		ni2dest = gen_rtx_REG (new_mode, REGNO (i2dest));
 	      else
 		{
-		  SUBST_MODE (regno_reg_rtx[REGNO (i2dest)], new_mode);
+		  subst_mode (REGNO (i2dest), new_mode);
 		  ni2dest = regno_reg_rtx[REGNO (i2dest)];
 		}
 
@@ -3712,7 +3711,7 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 		newdest = gen_rtx_REG (split_mode, REGNO (i2dest));
 	      else
 		{
-		  SUBST_MODE (regno_reg_rtx[REGNO (i2dest)], split_mode);
+		  subst_mode (REGNO (i2dest), split_mode);
 		  newdest = regno_reg_rtx[REGNO (i2dest)];
 		}
 	    }
@@ -4082,7 +4081,7 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
       for (undo = undobuf.undos; undo; undo = undo->next)
 	if (undo->kind == UNDO_MODE)
 	  {
-	    rtx reg = *undo->where.r;
+	    rtx reg = regno_reg_rtx[undo->where.regno];
 	    machine_mode new_mode = GET_MODE (reg);
 	    machine_mode old_mode = undo->old_contents.m;
 
@@ -4755,7 +4754,8 @@  undo_to_marker (void *marker)
 	  *undo->where.i = undo->old_contents.i;
 	  break;
 	case UNDO_MODE:
-	  adjust_reg_mode (*undo->where.r, undo->old_contents.m);
+	  adjust_reg_mode (regno_reg_rtx[undo->where.regno],
+			   undo->old_contents.m);
 	  break;
 	case UNDO_LINKS:
 	  *undo->where.l = undo->old_contents.l;
@@ -6819,7 +6819,7 @@  simplify_set (rtx x)
 		new_dest = gen_rtx_REG (compare_mode, regno);
 	      else
 		{
-		  SUBST_MODE (regno_reg_rtx[regno], compare_mode);
+		  subst_mode (regno, compare_mode);
 		  new_dest = regno_reg_rtx[regno];
 		}