Re: [RFC] Nonnegative improvements

[Roger Sayle <roger at eyesopen dot com>]

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.

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.

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:

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

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.


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.


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