[rtl] Harden 'set_noop_p' for non-constant selectors [PR94279] (was: [Patch, RTL] Eliminate redundant vec_select moves)

Message ID 87pnbztqru.fsf@euler.schwinge.homeip.net
State New
Headers show
Series
  • [rtl] Harden 'set_noop_p' for non-constant selectors [PR94279] (was: [Patch, RTL] Eliminate redundant vec_select moves)
Related show

Commit Message

Thomas Schwinge April 22, 2020, 4:43 p.m.
Hi!

First: please be gentle: I don't speak RTL.  ;-) And second: it's been
some time.

On 2013-12-04T16:06:48+0000, Tejas Belagod <tbelagod@arm.com> wrote:
> gcc/

>       * rtlanal.c (set_noop_p): Return nonzero in case of redundant vec_select

>       for overlapping register lanes.


This got committed to trunk in r205712.

> --- a/gcc/rtlanal.c

> +++ b/gcc/rtlanal.c

> @@ -1180,6 +1180,26 @@ set_noop_p (const_rtx set)

>        dst = SUBREG_REG (dst);

>      }

>

> +  /* It is a NOOP if destination overlaps with selected src vector

> +     elements.  */

> +  if (GET_CODE (src) == VEC_SELECT

> +      && REG_P (XEXP (src, 0)) && REG_P (dst)

> +      && HARD_REGISTER_P (XEXP (src, 0))

> +      && HARD_REGISTER_P (dst))

> +    {

> +      int i;

> +      rtx par = XEXP (src, 1);

> +      rtx src0 = XEXP (src, 0);

> +      int c0 = INTVAL (XVECEXP (par, 0, 0));

> +      HOST_WIDE_INT offset = GET_MODE_UNIT_SIZE (GET_MODE (src0)) * c0;

> +

> +      for (i = 1; i < XVECLEN (par, 0); i++)

> +     if (INTVAL (XVECEXP (par, 0, i)) != c0 + i)

> +       return 0;

> +      return simplify_subreg_regno (REGNO (src0), GET_MODE (src0),

> +                                 offset, GET_MODE (dst)) == (int)REGNO (dst);

> +    }

> +

>    return (REG_P (src) && REG_P (dst)

>         && REGNO (src) == REGNO (dst));

>  }


In <https://gcc.gnu.org/PR94279> "[amdgcn] internal compiler error: RTL
check: expected code 'const_int', have 'reg' in rtx_to_poly_int64, at
rtl.h:2379", we recently found that that it's wrong to expect constant
selectors, at least in the current code and its usage context.  (Thanks,
Richard Biener for the guidance!)  Not too many actually, but of course,
this code has seen some changes since 2013-12-04 (for example, r261530
"Use poly_int rtx accessors instead of hwi accessors"), and also the
context may have changed that it's being used in -- so, I'm not sure
whether the original code (as quoted above) is actually buggy already,
but it already does contain the pattern that 'INTVAL' is used on
something without making sure that we're actually dealing with a constant
selector.  (Has that maybe have been an impossible scenario back then?)

Anyway.  Attached is a WIP patch "[rtl] Harden 'set_noop_p' for
non-constant selectors [PR94279]".  Richard Biener said that "A patch
like along that line is pre-approved", but given my illiterateness with
what I'm deal with here, I'd like that reviewed properly, please.  :-)
If approving this patch, please respond with "Reviewed-by: NAME <EMAIL>"
so that your effort will be recorded in the commit log, see
<https://gcc.gnu.org/wiki/Reviewed-by>.

I'll schedule x86_64-pc-linux-gnu and powerpc64le-unknown-linux-gnu
bootstrap testing.  What other testing does this need?  (Asking as this
seems to have been added for aarch64, which I'm not set up to test.)  So
far, I've only confirmed that it does solve the RTL checking issue with
libgomp AMD GCN offloading testing.

Then, should this also be backported to release branches?  GCC 9: same
patch as for master branch.  GCC 8: pre poly_int, so only need to guard
'INTVAL' (by 'CONST_INT_P', right?).  Or, is that not worth it, given
that nobody found this to be a problem until now (as far as I know),
and/or it's maybe really specific to (or, exposed by) AMD GCN's vector
instructions?  (For AMD GCN offloading, we only care about master
branch.)


Grüße
 Thomas


-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter

Comments

Andrew Stubbs April 22, 2020, 5:01 p.m. | #1
On 22/04/2020 17:43, Thomas Schwinge wrote:
> In <https://gcc.gnu.org/PR94279> "[amdgcn] internal compiler error: RTL

> check: expected code 'const_int', have 'reg' in rtx_to_poly_int64, at

> rtl.h:2379", we recently found that that it's wrong to expect constant

> selectors, at least in the current code and its usage context.  (Thanks,

> Richard Biener for the guidance!)  Not too many actually, but of course,

> this code has seen some changes since 2013-12-04 (for example, r261530

> "Use poly_int rtx accessors instead of hwi accessors"), and also the

> context may have changed that it's being used in -- so, I'm not sure

> whether the original code (as quoted above) is actually buggy already,

> but it already does contain the pattern that 'INTVAL' is used on

> something without making sure that we're actually dealing with a constant

> selector.  (Has that maybe have been an impossible scenario back then?)


I think it was impossible. See 
https://gcc.gnu.org/legacy-ml/gcc-patches/2018-09/msg00273.html

> Then, should this also be backported to release branches?  GCC 9: same

> patch as for master branch.  GCC 8: pre poly_int, so only need to guard

> 'INTVAL' (by 'CONST_INT_P', right?).  Or, is that not worth it, given

> that nobody found this to be a problem until now (as far as I know),

> and/or it's maybe really specific to (or, exposed by) AMD GCN's vector

> instructions?  (For AMD GCN offloading, we only care about master

> branch.)


I don't think it's needed prior to GCC 9, and then only for amdgcn which 
was probably not widely used.

Andrew
Richard Sandiford April 22, 2020, 5:23 p.m. | #2
Andrew Stubbs <ams@codesourcery.com> writes:
> On 22/04/2020 17:43, Thomas Schwinge wrote:

>> In <https://gcc.gnu.org/PR94279> "[amdgcn] internal compiler error: RTL

>> check: expected code 'const_int', have 'reg' in rtx_to_poly_int64, at

>> rtl.h:2379", we recently found that that it's wrong to expect constant

>> selectors, at least in the current code and its usage context.  (Thanks,

>> Richard Biener for the guidance!)  Not too many actually, but of course,

>> this code has seen some changes since 2013-12-04 (for example, r261530

>> "Use poly_int rtx accessors instead of hwi accessors"), and also the

>> context may have changed that it's being used in -- so, I'm not sure

>> whether the original code (as quoted above) is actually buggy already,

>> but it already does contain the pattern that 'INTVAL' is used on

>> something without making sure that we're actually dealing with a constant

>> selector.  (Has that maybe have been an impossible scenario back then?)

>

> I think it was impossible. See 

> https://gcc.gnu.org/legacy-ml/gcc-patches/2018-09/msg00273.html


Ah!  Thanks for the link.

>> Then, should this also be backported to release branches?  GCC 9: same

>> patch as for master branch.  GCC 8: pre poly_int, so only need to guard

>> 'INTVAL' (by 'CONST_INT_P', right?).  Or, is that not worth it, given

>> that nobody found this to be a problem until now (as far as I know),

>> and/or it's maybe really specific to (or, exposed by) AMD GCN's vector

>> instructions?  (For AMD GCN offloading, we only care about master

>> branch.)

>

> I don't think it's needed prior to GCC 9, and then only for amdgcn which 

> was probably not widely used.


Based on that, OK for master and GCC 9.

Thanks,
Richard
Thomas Schwinge April 29, 2020, 8:44 a.m. | #3
Hi!

On 2020-04-22T18:23:24+0100, Richard Sandiford <richard.sandiford@arm.com> wrote:
> Andrew Stubbs <ams@codesourcery.com> writes:

>> On 22/04/2020 17:43, Thomas Schwinge wrote:

>>> In <https://gcc.gnu.org/PR94279> "[amdgcn] internal compiler error: RTL

>>> check: expected code 'const_int', have 'reg' in rtx_to_poly_int64, at

>>> rtl.h:2379", we recently found that that it's wrong to expect constant

>>> selectors, at least in the current code and its usage context.  (Thanks,

>>> Richard Biener for the guidance!)  Not too many actually, but of course,

>>> this code has seen some changes since 2013-12-04 (for example, r261530

>>> "Use poly_int rtx accessors instead of hwi accessors"), and also the

>>> context may have changed that it's being used in -- so, I'm not sure

>>> whether the original code (as quoted above) is actually buggy already,

>>> but it already does contain the pattern that 'INTVAL' is used on

>>> something without making sure that we're actually dealing with a constant

>>> selector.  (Has that maybe have been an impossible scenario back then?)

>>

>> I think it was impossible. See

>> https://gcc.gnu.org/legacy-ml/gcc-patches/2018-09/msg00273.html

>

> Ah!  Thanks for the link.


Many thanks indeed! That gives confidence why we're running into this
problem just now, for GCN target only -- Tejas' original patch thus is
not to blame at all, good.

(..., and hopefully we won't find much more fall-out due to the GCN
target doing away with the constant 'vec_select' restriction...)

>>> Then, should this also be backported to release branches?  GCC 9: same

>>> patch as for master branch.  GCC 8: pre poly_int, so only need to guard

>>> 'INTVAL' (by 'CONST_INT_P', right?).  Or, is that not worth it, given

>>> that nobody found this to be a problem until now (as far as I know),

>>> and/or it's maybe really specific to (or, exposed by) AMD GCN's vector

>>> instructions?  (For AMD GCN offloading, we only care about master

>>> branch.)

>>

>> I don't think it's needed prior to GCC 9, and then only for amdgcn which

>> was probably not widely used.

>

> Based on that, OK for master and GCC 9.


Thanks for the quick review.  I'll later see about backporting for GCC 9.
For now pushed to master branch in commit
f2c2eaaf8fb5c66ae372bb526b2b2fe67a9c5c39 "[rtl] Harden 'set_noop_p' for
non-constant selectors [PR94279]", see attached.


Grüße
 Thomas


-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
From f2c2eaaf8fb5c66ae372bb526b2b2fe67a9c5c39 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>

Date: Wed, 22 Apr 2020 16:58:44 +0200
Subject: [PATCH] [rtl] Harden 'set_noop_p' for non-constant selectors
 [PR94279]

... given that the GCN target did away with the constant 'vec_select'
restriction.

	gcc/
	PR target/94279
	* rtlanal.c (set_noop_p): Handle non-constant selectors.
---
 gcc/ChangeLog |  3 +++
 gcc/rtlanal.c | 12 +++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 2ba39f67200f..ef851ef84626 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,8 @@
 2020-04-29  Thomas Schwinge  <thomas@codesourcery.com>
 
+	PR target/94279
+	* rtlanal.c (set_noop_p): Handle non-constant selectors.
+
 	PR target/94282
 	* common/config/gcn/gcn-common.c (gcn_except_unwind_info): New
 	function.
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index c7ab86e228b1..0ebde7622db6 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -1631,12 +1631,18 @@ set_noop_p (const_rtx set)
       int i;
       rtx par = XEXP (src, 1);
       rtx src0 = XEXP (src, 0);
-      poly_int64 c0 = rtx_to_poly_int64 (XVECEXP (par, 0, 0));
+      poly_int64 c0;
+      if (!poly_int_rtx_p (XVECEXP (par, 0, 0), &c0))
+	return 0;
       poly_int64 offset = GET_MODE_UNIT_SIZE (GET_MODE (src0)) * c0;
 
       for (i = 1; i < XVECLEN (par, 0); i++)
-	if (maybe_ne (rtx_to_poly_int64 (XVECEXP (par, 0, i)), c0 + i))
-	  return 0;
+	{
+	  poly_int64 c0i;
+	  if (!poly_int_rtx_p (XVECEXP (par, 0, i), &c0i)
+	      || maybe_ne (c0i, c0 + i))
+	    return 0;
+	}
       return
 	REG_CAN_CHANGE_MODE_P (REGNO (dst), GET_MODE (src0), GET_MODE (dst))
 	&& simplify_subreg_regno (REGNO (src0), GET_MODE (src0),
-- 
2.26.2

Patch

From 3546ac8ef47cf67570834e5a70614907bef40304 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Wed, 22 Apr 2020 16:58:44 +0200
Subject: [PATCH] [rtl] Harden 'set_noop_p' for non-constant selectors
 [PR94279]

---
 gcc/rtlanal.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index c7ab86e228b1..0ebde7622db6 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -1631,12 +1631,18 @@  set_noop_p (const_rtx set)
       int i;
       rtx par = XEXP (src, 1);
       rtx src0 = XEXP (src, 0);
-      poly_int64 c0 = rtx_to_poly_int64 (XVECEXP (par, 0, 0));
+      poly_int64 c0;
+      if (!poly_int_rtx_p (XVECEXP (par, 0, 0), &c0))
+	return 0;
       poly_int64 offset = GET_MODE_UNIT_SIZE (GET_MODE (src0)) * c0;
 
       for (i = 1; i < XVECLEN (par, 0); i++)
-	if (maybe_ne (rtx_to_poly_int64 (XVECEXP (par, 0, i)), c0 + i))
-	  return 0;
+	{
+	  poly_int64 c0i;
+	  if (!poly_int_rtx_p (XVECEXP (par, 0, i), &c0i)
+	      || maybe_ne (c0i, c0 + i))
+	    return 0;
+	}
       return
 	REG_CAN_CHANGE_MODE_P (REGNO (dst), GET_MODE (src0), GET_MODE (dst))
 	&& simplify_subreg_regno (REGNO (src0), GET_MODE (src0),
-- 
2.25.1