Tighten check for vector types in fold_convertible_p (PR 92741)

Message ID mptfti2r8va.fsf@arm.com
State New
Headers show
Series
  • Tighten check for vector types in fold_convertible_p (PR 92741)
Related show

Commit Message

Richard Sandiford Dec. 2, 2019, 4:27 p.m.
In this PR, IPA-CP was misled into using NOP_EXPR rather than
VIEW_CONVERT_EXPR to reinterpret a vector of 4 shorts as a vector
of 2 ints.  This tripped the tree-cfg.c assert I'd added in r278245.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Richard


2019-12-02  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	PR middle-end/92741
	* fold-const.c (fold_convertible_p): Check vector types more
	thoroughly.

gcc/testsuite/
	PR middle-end/92741
	* gcc.dg/pr92741.c: New test.

Comments

Richard Biener Dec. 2, 2019, 4:32 p.m. | #1
On December 2, 2019 5:27:05 PM GMT+01:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
>In this PR, IPA-CP was misled into using NOP_EXPR rather than

>VIEW_CONVERT_EXPR to reinterpret a vector of 4 shorts as a vector

>of 2 ints.  This tripped the tree-cfg.c assert I'd added in r278245.

>

>Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?


Hmm, but then it may create such conversion without verifying the target supports it? 

Richard. 

>Richard

>

>

>2019-12-02  Richard Sandiford  <richard.sandiford@arm.com>

>

>gcc/

>	PR middle-end/92741

>	* fold-const.c (fold_convertible_p): Check vector types more

>	thoroughly.

>

>gcc/testsuite/

>	PR middle-end/92741

>	* gcc.dg/pr92741.c: New test.

>

>Index: gcc/fold-const.c

>===================================================================

>--- gcc/fold-const.c	2019-11-18 15:27:36.053367794 +0000

>+++ gcc/fold-const.c	2019-12-02 16:25:02.662405351 +0000

>@@ -2375,10 +2375,15 @@ fold_convertible_p (const_tree type, con

> 

>     case REAL_TYPE:

>     case FIXED_POINT_TYPE:

>-    case VECTOR_TYPE:

>     case VOID_TYPE:

>       return TREE_CODE (type) == TREE_CODE (orig);

> 

>+    case VECTOR_TYPE:

>+      return (VECTOR_TYPE_P (orig)

>+	      && known_eq (TYPE_VECTOR_SUBPARTS (type),

>+			   TYPE_VECTOR_SUBPARTS (orig))

>+	      && fold_convertible_p (TREE_TYPE (type), TREE_TYPE (orig)));

>+

>     default:

>       return false;

>     }

>Index: gcc/testsuite/gcc.dg/pr92741.c

>===================================================================

>--- /dev/null	2019-09-17 11:41:18.176664108 +0100

>+++ gcc/testsuite/gcc.dg/pr92741.c	2019-12-02 16:25:02.662405351 +0000

>@@ -0,0 +1,19 @@

>+/* { dg-options "-O2 -fexceptions -fnon-call-exceptions -fno-inline" }

>*/

>+

>+typedef int vh __attribute__ ((__vector_size__ (2 * sizeof (int))));

>+typedef short int cq __attribute__ ((__vector_size__ (4 * sizeof

>(short int))));

>+

>+static void

>+id (int *r8, vh *tu)

>+{

>+  *(vh *) r8 = *tu;

>+}

>+

>+void

>+mr (void)

>+{

>+  int r8;

>+  cq he = { 0, };

>+

>+  id (&r8, (vh *) &he);

>+}
Richard Sandiford Dec. 2, 2019, 4:40 p.m. | #2
Richard Biener <richard.guenther@gmail.com> writes:
> On December 2, 2019 5:27:05 PM GMT+01:00, Richard Sandiford <richard.sandiford@arm.com> wrote:

>>In this PR, IPA-CP was misled into using NOP_EXPR rather than

>>VIEW_CONVERT_EXPR to reinterpret a vector of 4 shorts as a vector

>>of 2 ints.  This tripped the tree-cfg.c assert I'd added in r278245.

>>

>>Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

>

> Hmm, but then it may create such conversion without verifying the target supports it? 


This doesn't seem like the right place to check that though.
E.g. tree-vect-generic.c could lower conversions if necessary, just
like for any other vector op.  AIUI fold_convertible_p is just checking
whether the conversion satisfies the tree/gimple semantics of NOP_EXPR.

Note that in the IPA-CP case, we should never generate a vector NOP_EXPR
that isn't also tree_nop_conversion_p, see comment 4 in the PR.  It's
possible we might want to rewrite the IPA-CP (and other IPA conditions)
to be more explicit about this, but I think we want the patch either way.
All it does is disallow cases that were previously (wrongly) allowed.

Thanks,
Richard

>

> Richard. 

>

>>Richard

>>

>>

>>2019-12-02  Richard Sandiford  <richard.sandiford@arm.com>

>>

>>gcc/

>>	PR middle-end/92741

>>	* fold-const.c (fold_convertible_p): Check vector types more

>>	thoroughly.

>>

>>gcc/testsuite/

>>	PR middle-end/92741

>>	* gcc.dg/pr92741.c: New test.

>>

>>Index: gcc/fold-const.c

>>===================================================================

>>--- gcc/fold-const.c	2019-11-18 15:27:36.053367794 +0000

>>+++ gcc/fold-const.c	2019-12-02 16:25:02.662405351 +0000

>>@@ -2375,10 +2375,15 @@ fold_convertible_p (const_tree type, con

>> 

>>     case REAL_TYPE:

>>     case FIXED_POINT_TYPE:

>>-    case VECTOR_TYPE:

>>     case VOID_TYPE:

>>       return TREE_CODE (type) == TREE_CODE (orig);

>> 

>>+    case VECTOR_TYPE:

>>+      return (VECTOR_TYPE_P (orig)

>>+	      && known_eq (TYPE_VECTOR_SUBPARTS (type),

>>+			   TYPE_VECTOR_SUBPARTS (orig))

>>+	      && fold_convertible_p (TREE_TYPE (type), TREE_TYPE (orig)));

>>+

>>     default:

>>       return false;

>>     }

>>Index: gcc/testsuite/gcc.dg/pr92741.c

>>===================================================================

>>--- /dev/null	2019-09-17 11:41:18.176664108 +0100

>>+++ gcc/testsuite/gcc.dg/pr92741.c	2019-12-02 16:25:02.662405351 +0000

>>@@ -0,0 +1,19 @@

>>+/* { dg-options "-O2 -fexceptions -fnon-call-exceptions -fno-inline" }

>>*/

>>+

>>+typedef int vh __attribute__ ((__vector_size__ (2 * sizeof (int))));

>>+typedef short int cq __attribute__ ((__vector_size__ (4 * sizeof

>>(short int))));

>>+

>>+static void

>>+id (int *r8, vh *tu)

>>+{

>>+  *(vh *) r8 = *tu;

>>+}

>>+

>>+void

>>+mr (void)

>>+{

>>+  int r8;

>>+  cq he = { 0, };

>>+

>>+  id (&r8, (vh *) &he);

>>+}
Richard Biener Dec. 2, 2019, 5:24 p.m. | #3
On December 2, 2019 5:40:41 PM GMT+01:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
>Richard Biener <richard.guenther@gmail.com> writes:

>> On December 2, 2019 5:27:05 PM GMT+01:00, Richard Sandiford

><richard.sandiford@arm.com> wrote:

>>>In this PR, IPA-CP was misled into using NOP_EXPR rather than

>>>VIEW_CONVERT_EXPR to reinterpret a vector of 4 shorts as a vector

>>>of 2 ints.  This tripped the tree-cfg.c assert I'd added in r278245.

>>>

>>>Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

>>

>> Hmm, but then it may create such conversion without verifying the

>target supports it? 

>

>This doesn't seem like the right place to check that though.


Agreed. 

>E.g. tree-vect-generic.c could lower conversions if necessary, just

>like for any other vector op.  AIUI fold_convertible_p is just checking

>whether the conversion satisfies the tree/gimple semantics of NOP_EXPR.


Hmm, OK. 

>Note that in the IPA-CP case, we should never generate a vector

>NOP_EXPR

>that isn't also tree_nop_conversion_p, see comment 4 in the PR.  It's

>possible we might want to rewrite the IPA-CP (and other IPA conditions)

>to be more explicit about this, but I think we want the patch either

>way.

>All it does is disallow cases that were previously (wrongly) allowed.


So let's go with the patch as is then. 

Thanks, 
Richard. 

>Thanks,

>Richard

>

>>

>> Richard. 

>>

>>>Richard

>>>

>>>

>>>2019-12-02  Richard Sandiford  <richard.sandiford@arm.com>

>>>

>>>gcc/

>>>	PR middle-end/92741

>>>	* fold-const.c (fold_convertible_p): Check vector types more

>>>	thoroughly.

>>>

>>>gcc/testsuite/

>>>	PR middle-end/92741

>>>	* gcc.dg/pr92741.c: New test.

>>>

>>>Index: gcc/fold-const.c

>>>===================================================================

>>>--- gcc/fold-const.c	2019-11-18 15:27:36.053367794 +0000

>>>+++ gcc/fold-const.c	2019-12-02 16:25:02.662405351 +0000

>>>@@ -2375,10 +2375,15 @@ fold_convertible_p (const_tree type, con

>>> 

>>>     case REAL_TYPE:

>>>     case FIXED_POINT_TYPE:

>>>-    case VECTOR_TYPE:

>>>     case VOID_TYPE:

>>>       return TREE_CODE (type) == TREE_CODE (orig);

>>> 

>>>+    case VECTOR_TYPE:

>>>+      return (VECTOR_TYPE_P (orig)

>>>+	      && known_eq (TYPE_VECTOR_SUBPARTS (type),

>>>+			   TYPE_VECTOR_SUBPARTS (orig))

>>>+	      && fold_convertible_p (TREE_TYPE (type), TREE_TYPE (orig)));

>>>+

>>>     default:

>>>       return false;

>>>     }

>>>Index: gcc/testsuite/gcc.dg/pr92741.c

>>>===================================================================

>>>--- /dev/null	2019-09-17 11:41:18.176664108 +0100

>>>+++ gcc/testsuite/gcc.dg/pr92741.c	2019-12-02 16:25:02.662405351

>+0000

>>>@@ -0,0 +1,19 @@

>>>+/* { dg-options "-O2 -fexceptions -fnon-call-exceptions -fno-inline"

>}

>>>*/

>>>+

>>>+typedef int vh __attribute__ ((__vector_size__ (2 * sizeof (int))));

>>>+typedef short int cq __attribute__ ((__vector_size__ (4 * sizeof

>>>(short int))));

>>>+

>>>+static void

>>>+id (int *r8, vh *tu)

>>>+{

>>>+  *(vh *) r8 = *tu;

>>>+}

>>>+

>>>+void

>>>+mr (void)

>>>+{

>>>+  int r8;

>>>+  cq he = { 0, };

>>>+

>>>+  id (&r8, (vh *) &he);

>>>+}

Patch

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	2019-11-18 15:27:36.053367794 +0000
+++ gcc/fold-const.c	2019-12-02 16:25:02.662405351 +0000
@@ -2375,10 +2375,15 @@  fold_convertible_p (const_tree type, con
 
     case REAL_TYPE:
     case FIXED_POINT_TYPE:
-    case VECTOR_TYPE:
     case VOID_TYPE:
       return TREE_CODE (type) == TREE_CODE (orig);
 
+    case VECTOR_TYPE:
+      return (VECTOR_TYPE_P (orig)
+	      && known_eq (TYPE_VECTOR_SUBPARTS (type),
+			   TYPE_VECTOR_SUBPARTS (orig))
+	      && fold_convertible_p (TREE_TYPE (type), TREE_TYPE (orig)));
+
     default:
       return false;
     }
Index: gcc/testsuite/gcc.dg/pr92741.c
===================================================================
--- /dev/null	2019-09-17 11:41:18.176664108 +0100
+++ gcc/testsuite/gcc.dg/pr92741.c	2019-12-02 16:25:02.662405351 +0000
@@ -0,0 +1,19 @@ 
+/* { dg-options "-O2 -fexceptions -fnon-call-exceptions -fno-inline" } */
+
+typedef int vh __attribute__ ((__vector_size__ (2 * sizeof (int))));
+typedef short int cq __attribute__ ((__vector_size__ (4 * sizeof (short int))));
+
+static void
+id (int *r8, vh *tu)
+{
+  *(vh *) r8 = *tu;
+}
+
+void
+mr (void)
+{
+  int r8;
+  cq he = { 0, };
+
+  id (&r8, (vh *) &he);
+}