Re: [RFC] Nonnegative improvements

[ja2morri at csclub dot uwaterloo dot ca (James A. Morrison)]

Roger Sayle <roger@eyesopen.com> writes:

> On 19 Jun 2005, James A. Morrison wrote:
> >   I'm looking for comments about hooking value range data into the
> > simplification code, starting with a nonnegative property.  The attached
> > patch has seen some light testing, but I'm looking for comments about the
> > approach.
> 
> Whilst I think this is a step we want to take, I would however proceed
> extremely cautiously.  Firstly, I recommend that you split your proposed
> patch into it's two/three independent parts, and fully bootstrap and
> regression test each independently.  "some light testing" is probably
> right out!  In fact, given the bad interactions between VRP and the
> Ada front-end, it might even make sense to wait until Ada is testable
> again or we return to stage1.

 Yes, I certainly wasn't trying to get this patch in given how little testing
I've done with it.
 
> The first independent bit is the TYPE_UNSIGNED change to the top
> of tree_expr_nonnegative_p.  I think this is should be OK, but needs
> to be tested.

  I hope so, this change may have be causing me trouble though.

> The second independent bit is the SSA_NAME change to that function
> and the new vrp_nonnegative_p function.  I like the way you use
> (slightly misuse) the existing vr_value global variable to indicate
> that range information is available, and explicitly reset it to
> zero when in it no longer is.  However, the following change could
> be better written:

 Thanks, I think vr_value is a good indicator if range information is
available.

> +int
> +vrp_nonnegative_p (tree t)
> +{
> +  tree val;
> +  value_range_t *vr;
> +
> +  if (!vr_value)
> +    return 0;
> +
> +  vr = get_value_range (t);
> +  if (TYPE_UNSIGNED (TREE_TYPE (t)))
> +    val = integer_one_node;
> +  else
> +    val = compare_range_with_value (GE_EXPR, vr, integer_zero_node);
> +
> +  return val && integer_onep (val);
> +}
> 
> 
> You can test for TYPE_UNSIGNED before get_value_range and even before
> testing vr_value, and immediately return "true".  The return type of
> the function should be bool.  There's no reason to use integer_one_node
> and then test whether it's NULL and integer_onep other than this idiom
> was cut'n'paste from elsewhere.

 The TYPE_UNSIGNED check should be removeable if TYPE_UNSIGNED is always
checked outside vrp_nonnegative_p.  The reason val needs to be checked for
NULL is that compare_range_with_value can return NULL.  Otherwise, you are
right, returning immediatly when TYPE_UNSIGNED is true makes much more sense
that what I have above.

> Finally, the last "enabling" change to simplify_using ranges isn't
> independent but could/should be tested independently.  Instead of
> "continue" at this point, I'd suggest that you simply update "rhs"
> and "rhs_code", and fall through to the existing optimizations.
> This defends against fold making trivial changes, but once some more
> range-sensitive optimizations are added to this function, we might
> reach the point where fold simplifies "x" to "y", and the resulting
> "y" can further be simplified with range information.

 Nice catch.  When I am happy with the vrp_nonnegative_p bit, I will separate
this out and submit this as a separate patch.
 
> One reason for my concerns is the different middle-end type systems
> used by fold vs. the tree-ssa optimizers.  In fold, types should
> always be perfectly preserved (correct), whilst in SSA form it is OK
> to omit "useless" type conversions.  It's the fact that useless type
> conversions include signedness and range changes that are one cause
> of VRP's current instability.  I believe bridging the two worlds may
> be safe, provided that we have the invariant that useless conversions
> are only omitted from the top-level RHS of MODIFY_EXPRs, i.e. are
> implictly represented by the assignment but the expression trees
> themselves are well formed.  The correct type can be inferred from
> the TREE_TYPE (lhs).
> 
> Hence in "ssa_name = rhs;" its often the case (due to useless conversions)
> that tree_expr_nonnegative_p(rhs) != tree_expr_nonnegative_p(ssa_name),
> and vr_range (rhs) != vr_range (lhs), which can be confusing to VRP.

 Ug, this makes me cringe.

> Once again, I like your solution of how to hook into VRP information
> when available.  I've intentionally been holding off refactoring
> something like tree_expr_minval and tree_expr_maxval in fold-const.c
> due to the obvious overlap with VRP and no clear plan on how to link
> the two.  The use of vr_value avoids the need for hooks, or similar
> infrastructure.  However, perhaps an additional comment above the
> declaration of vr_value describing it's extended semantics would be
> in order, provided this approach isn't considered too ugly by other
> maintainers.
> 
> Roger

 I hope it isn't too ugly.  This is what I gathered Jeff wanted from his
comments in the TRUNC_MOD_EXPR thread.

-- 
Thanks,
Jim

http://www.csclub.uwaterloo.ca/~ja2morri/
http://phython.blogspot.com
http://open.nit.ca/wiki/?page=jim