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

[Richard Biener]

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.  
*/

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

[Marc Glisse]

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

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

[Richard Biener]

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.

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

[Richard Biener]

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  
> 
>   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 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)

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

[Marc Glisse]

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

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

[Jakub Jelinek]

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  

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