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