Re: [RFC][IPA-VRP] Early VRP Implementation

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Jul 15, 2016 at 9:33 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Andrew,
>
> On 15/07/16 17:28, Andrew Pinski wrote:
>>
>> On Fri, Jul 15, 2016 at 12:08 AM, kugan
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>
>>> Hi Andrew,
>>>
>>>> Why separate out early VRP from tree-vrp?  Just a little curious.
>>>
>>>
>>>
>>> It is based on the discussion in
>>> https://gcc.gnu.org/ml/gcc/2016-01/msg00069.html.
>>> In summary, conclusion (based on my understanding) was to implement a
>>> simplified VRP algorithm that doesn't require ASSERT_EXPR insertion.
>>
>>
>> But I don't see why you are moving it from tree-vrp.c .  That was my
>> question, you pointing to that discussion does not say to split it
>> into a new file and expose these interfaces.
>>
>
> Are you saying that I should keep this part of tree-vrp.c. I am happy to do
> that if this is considered the best approach.

Yes, I think that's the best approach.

Can you, as a refactoring before your patch, please change VRP to use
an alloc-pool
for allocating value_range?  The DOM-based VRP will add a lot of
malloc/free churn
otherwise.

Generally watch coding-style such as  missed function comments.

As you do a non-iterating VRP and thus do not visit back-edges you don't need
to initialize loops or SCEV nor do you need loop-closed SSA.

As you do a DOM-based VRP using SSA propagator stuff like ssa_prop_result
doesn't make any sense.

+edge evrp_dom_walker::before_dom_children (basic_block bb)
+{
+  /* If we are going out of scope, restore the old VR.  */
+  while (!cond_stack.is_empty ()
+        && !dominated_by_p (CDI_DOMINATORS, bb, cond_stack.last ().first))
+    {
+      tree var = cond_stack.last ().second.first;
+      value_range *vr = cond_stack.last ().second.second;
+      value_range *vr_to_del = get_value_range (var);
+      XDELETE (vr_to_del);
+      change_value_range (var, vr);
+      cond_stack.pop ();
+    }

that should be in after_dom_children I think and use a marker instead
of a DOM query.
See other examples like DOM itself or SCCVN.

+         /* Discover VR when condition is true.  */
+         if (te == e
+             && !TREE_OVERFLOW_P (op0)
+             && !TREE_OVERFLOW_P (op1))

you can use e->flags & EDGE_TRUE_VALUE/EDGE_FALSE_VALUE

why do you need those TREE_OVERFLOW checks?

+             tree cond = build2 (code, boolean_type_node, op0, op1);
+             tree a = build2 (ASSERT_EXPR, TREE_TYPE (op0), op0, cond);
+             extract_range_from_assert (&vr, a);

so I was hoping that the "refactoring" patch in the series would expose a more
useful interface than extract_range_from_assert ... namely one that can
extract a range from the comparison directly and does not require building
a scratch ASSERT_EXPR.

+         /* If we found any usable VR, set the VR to ssa_name and create a
+            restore point in the cond_stack with the  old VR. */
+         if (vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE)
+           {
+             value_range *new_vr = XCNEW (value_range);
+             *new_vr = vr;
+             cond_stack.safe_push (std::make_pair (bb,
+                                                   std::make_pair (op0,
+                                                                   old_vr)));
+             change_value_range (op0, new_vr);

I don't like 'change_value_range' as interface, please integrate that into
a push/pop_value_range style interface instead.

+       vrp_visit_stmt (stmt, &taken_edge_p, &output_p);
+    }
+
+  return NULL;

you should return taken_edge_p (misnamed as it isn't a pointer) and take
advantage of EDGE_EXECUTABLE.  Again see DOM/SCCVN (might want to
defer this as a followup improvement).

Note that the advantage of a DOM-based VRP is that backtracking is easy
to implement (you don't do that yet).  That is, after DEF got a (better)
value-range you can simply re-visit all the defs of its uses (and recursively).
I think you have to be careful with stmts that might prematurely leave a BB
though (like via EH).  So sth for a followup as well.

Thanks,
Richard.


> Thanks,
> Kugan