Re: [PATCH] Fix the constant mess in PRE

[Richard Guenther <rguenther at suse dot de>]

On Fri, 18 Jul 2008, Richard Guenther wrote:

> On Thu, 17 Jul 2008, Richard Guenther wrote:
> 
> > On Thu, 17 Jul 2008, Daniel Berlin wrote:
> > 
> > > On Thu, Jul 17, 2008 at 7:36 AM, Richard Guenther <rguenther@suse.de> wrote:
> > > > On Wed, 16 Jul 2008, Daniel Berlin wrote:
> > > >
> > > >> In any case, if your patch works without the other workarounds i had,
> > > >> i'd rather use your patch, since it would be nice to go back to
> > > >> constants being their own reps.
> > > >
> > > > I verified that reverting
> > > >
> > > >        2008-07-15  Daniel Berlin  <dberlin@dberlin.org>
> > > >
> > > >        * tree-ssa-sccvn.c (expressions_equal_p): Check type equality.
> > > >        * tree-ssa-pre.c (pre_expr_eq): Ditto
> > > >        (get_constant_for_value_id): Take a type as an argument.
> > > >        (fully_constant_expression): Pass in type.
> > > >        (find_or_generate_expression): Short circuit constant case.
> > > >        (create_expression_by_pieces): Remove special casing of
> > > >        pointer_plus.
> > > >        (do_regular_insertion): Short circuit constant case.
> > > >        (do_partial_partial_insertion): Ditto.
> > > >
> > > > and applying the patch to trunk only fixes some more regressions.
> > > >
> > > Great!
> > > 
> > > > Should we still keep the "Short circuit constant case" cases or will
> > > > that be fixed automatically by making constants represent themselves?
> > > 
> > > It should be fixed, but it would be ideal if we bootstrapped it once
> > > with an assert in those places like
> > > 
> > > gcc_assert(useless_type_conversion_p(TREE_TYPE (result from
> > > bitmap_find_leader), TREE_TYPE (expr we pass in)))
> > > .
> > > (In all those places you have the original expression a constant, and
> > > this should just check that the constant we get back has the same
> > > type).
> > > 
> > > Just to make sure it's not slipping through the cracks somehow.
> > > 
> > > I'd rather be finally done with this mess :)
> > > 
> > > I'm happy to do this and commit the result if it all works, if you are
> > > busy with tuples.
> > 
> > Btw, on tuples (with some more patches) I seem to need the following
> > (otherwise we ICE in get_constant_for_value_id sometimes).  I guess
> > it makes sense to shortcut here, but maybe there's still some latent
> > bug.
> 
> Which is, we do not fill in the expression-set for all constants.
> The following fixes it.  But I wonder why we need expression-sets
> for constants at all ...

And this one is for the problem you mentioned on IRC.  Did you
have a testcase for this one?

Thanks,
Richard.

2008-07-18  Richard Guenther  <rguenther@suse.de>

	* tree-ssa-sccvn.h (vn_constant_eq_with_type): Make sure an
	integral type is
	never equal to a non-integral type.
	(vn_hash_constant_with_type): Adjust.

Index: gcc/tree-ssa-sccvn.h
===================================================================
*** gcc/tree-ssa-sccvn.h.orig	2008-07-18 11:17:39.000000000 +0200
--- gcc/tree-ssa-sccvn.h	2008-07-18 11:27:29.000000000 +0200
*************** vn_hash_constant_with_type (tree constan
*** 113,118 ****
--- 113,119 ----
  {
    tree type = TREE_TYPE (constant);
    return (iterative_hash_expr (constant, 0)
+ 	  + INTEGRAL_TYPE_P (type)
  	  + (INTEGRAL_TYPE_P (type)
  	     ? TYPE_PRECISION (type) + TYPE_UNSIGNED (type) : 0));
  }
*************** vn_constant_eq_with_type (tree c1, tree
*** 126,131 ****
--- 127,133 ----
    tree type1 = TREE_TYPE (c1);
    tree type2 = TREE_TYPE (c2);
    return (expressions_equal_p (c1, c2)
+ 	  && INTEGRAL_TYPE_P (type1) == INTEGRAL_TYPE_P (type2)
  	  && (!INTEGRAL_TYPE_P (type1)
  	      || ((TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
  		  && (TYPE_UNSIGNED (type1) == TYPE_UNSIGNED (type2))))));