Fix (intptr_t) x eq/ne CST to x eq/ne (typeof x) cst match.pd pattern (PR tree-optimization/85446)

Message ID 20180418220836.GT8577@tucnak
State New
Headers show
Series
  • Fix (intptr_t) x eq/ne CST to x eq/ne (typeof x) cst match.pd pattern (PR tree-optimization/85446)
Related show

Commit Message

Jakub Jelinek April 18, 2018, 10:08 p.m.
Hi!

As mentioned in the PR, this optimization can't work if @0's precision
is higher than @1's precision, because originally it compares just some set
of lower bits, but in the new comparison compares all bits.
If @0's precision is smaller than @1's precision (in this case @0 can't be
a pointer, as we disallow such direct casts), then in theory it can be
handled, but will not match what the comment says and we'd need to verify
that the @1 constant can be represented in the @0's precision.

This patch just verifies the precision is the same and does small formatting
cleanup.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2018-04-18  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/85446
	* match.pd ((intptr_t) x eq/ne CST to x eq/ne (typeof x) cst): Require
	the integral and pointer types to have the same precision.


	Jakub

Comments

Marc Glisse April 18, 2018, 11:44 p.m. | #1
On Thu, 19 Apr 2018, Jakub Jelinek wrote:

> As mentioned in the PR, this optimization can't work if @0's precision

> is higher than @1's precision, because originally it compares just some set

> of lower bits, but in the new comparison compares all bits.

> If @0's precision is smaller than @1's precision (in this case @0 can't be

> a pointer, as we disallow such direct casts), then in theory it can be

> handled, but will not match what the comment says and we'd need to verify

> that the @1 constant can be represented in the @0's precision.

>

> This patch just verifies the precision is the same and does small formatting

> cleanup.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for

> trunk?


That certainly seems safe, but I am surprised to see a direct cast from 
64-bit pointer to 32-bit integer. I've always seen gcc represent those 
with an intermediate cast to a 64-bit integer, even if 
verify_gimple_assign_unary allows the direct cast. Does it depend on the 
platform? It might be nice to canonicalize this a bit, either by 
forbidding narrowing pointer-to-integer casts, or by simplifying cast 
chains to direct casts.

-- 
Marc Glisse
Richard Biener April 19, 2018, 7:08 a.m. | #2
On Thu, 19 Apr 2018, Jakub Jelinek wrote:

> Hi!

> 

> As mentioned in the PR, this optimization can't work if @0's precision

> is higher than @1's precision, because originally it compares just some set

> of lower bits, but in the new comparison compares all bits.

> If @0's precision is smaller than @1's precision (in this case @0 can't be

> a pointer, as we disallow such direct casts), then in theory it can be

> handled, but will not match what the comment says and we'd need to verify

> that the @1 constant can be represented in the @0's precision.

> 

> This patch just verifies the precision is the same and does small formatting

> cleanup.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for

> trunk?


OK.

Richard.

> 2018-04-18  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR tree-optimization/85446

> 	* match.pd ((intptr_t) x eq/ne CST to x eq/ne (typeof x) cst): Require

> 	the integral and pointer types to have the same precision.

> 

> --- gcc/match.pd.jj	2018-04-09 20:15:49.158631652 +0200

> +++ gcc/match.pd	2018-04-18 09:55:47.176343913 +0200

> @@ -3711,10 +3711,13 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)

>  (for cmp (ne eq)

>   (simplify

>    (cmp (convert @0) INTEGER_CST@1)

> -  (if ((POINTER_TYPE_P (TREE_TYPE (@0)) && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (@0)))

> -	&& INTEGRAL_TYPE_P (TREE_TYPE (@1)))

> -      || (INTEGRAL_TYPE_P (TREE_TYPE (@0)) && POINTER_TYPE_P (TREE_TYPE (@1))

> -	  && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (@1)))))

> +  (if (((POINTER_TYPE_P (TREE_TYPE (@0))

> +	 && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (@0)))

> +	 && INTEGRAL_TYPE_P (TREE_TYPE (@1)))

> +	|| (INTEGRAL_TYPE_P (TREE_TYPE (@0))

> +	    && POINTER_TYPE_P (TREE_TYPE (@1))

> +	    && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (@1)))))

> +       && TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1)))

>     (cmp @0 (convert @1)))))

>  

>  /* Non-equality compare simplifications from fold_binary  */

> 

> 	Jakub

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Richard Biener April 19, 2018, 7:22 a.m. | #3
On Thu, 19 Apr 2018, Marc Glisse wrote:

> On Thu, 19 Apr 2018, Jakub Jelinek wrote:

> 

> > As mentioned in the PR, this optimization can't work if @0's precision

> > is higher than @1's precision, because originally it compares just some set

> > of lower bits, but in the new comparison compares all bits.

> > If @0's precision is smaller than @1's precision (in this case @0 can't be

> > a pointer, as we disallow such direct casts), then in theory it can be

> > handled, but will not match what the comment says and we'd need to verify

> > that the @1 constant can be represented in the @0's precision.

> > 

> > This patch just verifies the precision is the same and does small formatting

> > cleanup.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for

> > trunk?

> 

> That certainly seems safe, but I am surprised to see a direct cast from 64-bit

> pointer to 32-bit integer. I've always seen gcc represent those with an

> intermediate cast to a 64-bit integer, even if verify_gimple_assign_unary

> allows the direct cast. Does it depend on the platform? It might be nice to

> canonicalize this a bit, either by forbidding narrowing pointer-to-integer

> casts, or by simplifying cast chains to direct casts.


We are only (well, that was the intention until I broke the verifier...)
disallowing widening casts from pointers because whether there is
zero- or sign-extension involved isn't specified (in fact TYPE_SIGN
of the pointer isn't what matters here but POINTERS_EXTEND_UNSIGNED,
and that's even not well-defined for random address-spaces I think).

Not sure if it's really required to restrict things further.

Richard.
Marc Glisse April 19, 2018, 10:47 a.m. | #4
On Thu, 19 Apr 2018, Richard Biener wrote:

> On Thu, 19 Apr 2018, Marc Glisse wrote:

>

>> On Thu, 19 Apr 2018, Jakub Jelinek wrote:

>>

>>> As mentioned in the PR, this optimization can't work if @0's precision

>>> is higher than @1's precision, because originally it compares just some set

>>> of lower bits, but in the new comparison compares all bits.

>>> If @0's precision is smaller than @1's precision (in this case @0 can't be

>>> a pointer, as we disallow such direct casts), then in theory it can be

>>> handled, but will not match what the comment says and we'd need to verify

>>> that the @1 constant can be represented in the @0's precision.

>>>

>>> This patch just verifies the precision is the same and does small formatting

>>> cleanup.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for

>>> trunk?

>>

>> That certainly seems safe, but I am surprised to see a direct cast from 64-bit

>> pointer to 32-bit integer. I've always seen gcc represent those with an

>> intermediate cast to a 64-bit integer, even if verify_gimple_assign_unary

>> allows the direct cast. Does it depend on the platform? It might be nice to

>> canonicalize this a bit, either by forbidding narrowing pointer-to-integer

>> casts, or by simplifying cast chains to direct casts.

>

> We are only (well, that was the intention until I broke the verifier...)

> disallowing widening casts from pointers because whether there is

> zero- or sign-extension involved isn't specified (in fact TYPE_SIGN

> of the pointer isn't what matters here but POINTERS_EXTEND_UNSIGNED,

> and that's even not well-defined for random address-spaces I think).

>

> Not sure if it's really required to restrict things further.


Then we should probably go with option 2 "simplifying cast chains to 
direct casts". Currently,

   unsigned f(char*p){return p;}

is turned into

   p.0_1 = (long int) p_2(D);
   _3 = (unsigned int) p.0_1;

instead of the simpler (more canonical?)

   _3 = (unsigned int) p_2(D);

(ideally to me, the type should be part of the operations more than the 
objects, so "p.0_1 = (long int) p_2(D)" would just be a copy and not a 
(nop) conversion, but that would be way too big a change)

-- 
Marc Glisse
Richard Biener April 19, 2018, 10:51 a.m. | #5
On Thu, 19 Apr 2018, Marc Glisse wrote:

> On Thu, 19 Apr 2018, Richard Biener wrote:

> 

> > On Thu, 19 Apr 2018, Marc Glisse wrote:

> > 

> > > On Thu, 19 Apr 2018, Jakub Jelinek wrote:

> > > 

> > > > As mentioned in the PR, this optimization can't work if @0's precision

> > > > is higher than @1's precision, because originally it compares just some

> > > > set

> > > > of lower bits, but in the new comparison compares all bits.

> > > > If @0's precision is smaller than @1's precision (in this case @0 can't

> > > > be

> > > > a pointer, as we disallow such direct casts), then in theory it can be

> > > > handled, but will not match what the comment says and we'd need to

> > > > verify

> > > > that the @1 constant can be represented in the @0's precision.

> > > > 

> > > > This patch just verifies the precision is the same and does small

> > > > formatting

> > > > cleanup.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for

> > > > trunk?

> > > 

> > > That certainly seems safe, but I am surprised to see a direct cast from

> > > 64-bit

> > > pointer to 32-bit integer. I've always seen gcc represent those with an

> > > intermediate cast to a 64-bit integer, even if verify_gimple_assign_unary

> > > allows the direct cast. Does it depend on the platform? It might be nice

> > > to

> > > canonicalize this a bit, either by forbidding narrowing pointer-to-integer

> > > casts, or by simplifying cast chains to direct casts.

> > 

> > We are only (well, that was the intention until I broke the verifier...)

> > disallowing widening casts from pointers because whether there is

> > zero- or sign-extension involved isn't specified (in fact TYPE_SIGN

> > of the pointer isn't what matters here but POINTERS_EXTEND_UNSIGNED,

> > and that's even not well-defined for random address-spaces I think).

> > 

> > Not sure if it's really required to restrict things further.

> 

> Then we should probably go with option 2 "simplifying cast chains to direct

> casts". Currently,

> 

>   unsigned f(char*p){return p;}

> 

> is turned into

> 

>   p.0_1 = (long int) p_2(D);

>   _3 = (unsigned int) p.0_1;

> 

> instead of the simpler (more canonical?)

> 

>   _3 = (unsigned int) p_2(D);


Yes.  Probably some restriction in a folder that tries to implement
a more strict pointer vs. integer separation than what is currently
enforced by the GIMPLE verifier which still needs the fix below.
[ideally we'd also close that ptrofftype_p loop-hole...]

> (ideally to me, the type should be part of the operations more than the

> objects, so "p.0_1 = (long int) p_2(D)" would just be a copy and not a (nop)

> conversion, but that would be way too big a change)


Yeah...

Richard.

Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c      (revision 259457)
+++ gcc/tree-cfg.c      (working copy)
@@ -3842,7 +3842,7 @@ verify_gimple_assign_unary (gassign *stm
            || (POINTER_TYPE_P (rhs1_type)
                && INTEGRAL_TYPE_P (lhs_type)
                && (TYPE_PRECISION (rhs1_type) >= TYPE_PRECISION (lhs_type)
-                   || ptrofftype_p (sizetype))))
+                   || ptrofftype_p (lhs_type))))
          return false;
 
        /* Allow conversion from integral to offset type and vice versa.  
*/

Patch

--- gcc/match.pd.jj	2018-04-09 20:15:49.158631652 +0200
+++ gcc/match.pd	2018-04-18 09:55:47.176343913 +0200
@@ -3711,10 +3711,13 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 (for cmp (ne eq)
  (simplify
   (cmp (convert @0) INTEGER_CST@1)
-  (if ((POINTER_TYPE_P (TREE_TYPE (@0)) && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (@0)))
-	&& INTEGRAL_TYPE_P (TREE_TYPE (@1)))
-      || (INTEGRAL_TYPE_P (TREE_TYPE (@0)) && POINTER_TYPE_P (TREE_TYPE (@1))
-	  && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (@1)))))
+  (if (((POINTER_TYPE_P (TREE_TYPE (@0))
+	 && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (@0)))
+	 && INTEGRAL_TYPE_P (TREE_TYPE (@1)))
+	|| (INTEGRAL_TYPE_P (TREE_TYPE (@0))
+	    && POINTER_TYPE_P (TREE_TYPE (@1))
+	    && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (@1)))))
+       && TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1)))
    (cmp @0 (convert @1)))))
 
 /* Non-equality compare simplifications from fold_binary  */