On Fri, Jul 15, 2016 at 9:33 AM, kugan
<kugan.vivekanandara...@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.vivekanandara...@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
