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

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

[Jakub Jelinek]

Hi!

The testcase in the PR fails under valgrind on mips64 (but only Martin
can reproduce, I couldn't).
But the problem reported there is that SUBST_MODE remembers addresses
into the regno_reg_rtx array, then some splitter needs a new pseudo
and calls gen_reg_rtx, which reallocates the regno_reg_rtx array
and finally undo operation is done and dereferences the old regno_reg_rtx
entry.
The rtx values stored in regno_reg_rtx array seems to be created
by gen_reg_rtx only and since then aren't modified, all we do for it
is adjusting its fields (e.g. adjust_reg_mode that SUBST_MODE does).

So, I think it is useless to use where.r for UNDO_MODE and store
&regno_reg_rtx[regno] in struct undo, we can store just
regno_reg_rtx[regno] (i.e. pointer to the REG itself instead of
pointer to pointer to REG) or could also store just the regno.

The following patch does the former.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

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.
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))

2022-03-29  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/104985
	* combine.cc (struct undo): Add where.m member.
	(do_SUBST_MODE): Rename to ...
	(SUBST_MODE): ... this.  No longer a macro.  Change first argument
	from rtx * into rtx, operate on INTO rather than *INTO and save
	INTO into where.m.
	(try_combine, undo_to_marker): For UNDO_MODE, use undo->where.m
	instead of *undo->where.r.

--- gcc/combine.cc.jj	2022-02-16 14:48:23.847681860 +0100
+++ gcc/combine.cc	2022-03-28 14:37:04.708490651 +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; rtx m; struct insn_link **l; } where;
 };
 
 /* Record a bunch of changes to be undone, up to MAX_UNDO of them.
@@ -761,10 +761,10 @@ do_SUBST_INT (int *into, int newval)
    well.  */
 
 static void
-do_SUBST_MODE (rtx *into, machine_mode newval)
+SUBST_MODE (rtx into, machine_mode newval)
 {
   struct undo *buf;
-  machine_mode oldval = GET_MODE (*into);
+  machine_mode oldval = GET_MODE (into);
 
   if (oldval == newval)
     return;
@@ -775,15 +775,13 @@ do_SUBST_MODE (rtx *into, machine_mode n
     buf = XNEW (struct undo);
 
   buf->kind = UNDO_MODE;
-  buf->where.r = into;
+  buf->where.m = into;
   buf->old_contents.m = oldval;
-  adjust_reg_mode (*into, newval);
+  adjust_reg_mode (into, 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
@@ -4082,7 +4080,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 = undo->where.m;
 	    machine_mode new_mode = GET_MODE (reg);
 	    machine_mode old_mode = undo->old_contents.m;
 
@@ -4755,7 +4753,7 @@ 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 (undo->where.m, undo->old_contents.m);
 	  break;
 	case UNDO_LINKS:
 	  *undo->where.l = undo->old_contents.l;

	Jakub

Re: [PATCH] combine: Don't record for UNDO_MODE pointers into regno_reg_rtx array [PR104985]

[Segher Boessenkool]

Hi!

On Tue, Mar 29, 2022 at 09:52:43AM +0200, Jakub Jelinek wrote:
> The testcase in the PR fails under valgrind on mips64 (but only Martin
> can reproduce, I couldn't).
> But the problem reported there is that SUBST_MODE remembers addresses
> into the regno_reg_rtx array, then some splitter needs a new pseudo
> and calls gen_reg_rtx, which reallocates the regno_reg_rtx array
> and finally undo operation is done and dereferences the old regno_reg_rtx
> entry.
> The rtx values stored in regno_reg_rtx array seems to be created
> by gen_reg_rtx only and since then aren't modified, all we do for it
> is adjusting its fields (e.g. adjust_reg_mode that SUBST_MODE does).

Yes, every pseudo has one and only one mode.  If you make sure every use
of the pseudo can have that new mode, this works (as opposed to with
hard registers, where you need more checks).

> So, I think it is useless to use where.r for UNDO_MODE and store
> &regno_reg_rtx[regno] in struct undo, we can store just
> regno_reg_rtx[regno] (i.e. pointer to the REG itself instead of
> pointer to pointer to REG) or could also store just the regno.

That should work yes.

> 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?

Okay for trunk with those things fixed.  Thanks!


Segher

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

[Jakub Jelinek]

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.

--- 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];
 		}
 


	Jakub

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

[Segher Boessenkool]

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