diff mbox series

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

Commit Message

Richard Sandiford Dec. 2, 2019, 4:27 p.m. UTC 
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. UTC | #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. UTC | #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. UTC | #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);
>>>+}
diff mbox series

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);
+}