On 10/26/2017 12:11 PM, Richard Biener wrote:
> On October 26, 2017 6:50:15 PM GMT+02:00, Jeff Law <l...@redhat.com> wrote:
[ Big snip ]
>>> Both patches look ok to me though it would be nice to
>>> do the actual composition with a comment that the
>>> lattices might be moved here (if all workers became
>>> member functions as well).
>> I'm actually hoping to post a snapshot of how this will look in a clean
>> consumer today (sprintf bits). There's enough in place that I can do
>> that while I continue teasing apart vrp and evrp.
>
> Good. Nice to see we're in the same boat.
And just so folks can see where this is going. This is what it takes to
embed a vrp-style range analyzer into the sprintf warnings pass.
There's a #include to pick up the range analyzer. A new override in the
sprintf dom walker and a new data member in the sprintf dom walker.
You can see the calls into the range analyzer, one in the
before_dom_children override and the other in the after_dom_children
override to generate the context sensitive range information. There's 3
calls into a class method to get the range info.
That's the general procedure for any dom walker that we'd want to have
access to context sensitive range data. #include, new data member in
the walker class, an enter/exit call in the {before,after}_dom_children
overrides and redirecting the queries into the range analyzer rather
than querying the global info.
Two warts in the current implementation:
First, the queries into the range analyzer occur deeper into the call
chain, so we don't have trivial access to the analyzer class instance.
So for now I just shove it into a global.
This is a common issue I see with our codebase -- we may have some of
the higher level objects in place, but when it comes to accessing those
objects deeper in the call chains, we punt to globals. It's probably as
much an artifact of our history as a C project as anything.
Truly class-ifying the code or passing down the class instance sometimes
has little/no benefit and we stop and punt to a global. That's probably
not an unreasonable decision, particularly in self-contained code. But
once we want to re-use the code it's a major hindrance. In fact, the
whole process around cleaning up vr_data in tree-vrp is another instance
of this exact issue. Anyway, I'll try to avoid ranting too much
because I'm as guilty as anyone on this issue.
Second. The name of the method to get the range information from the
analyzer is the same as the global scoped function to get range
information from the SSA_NAME. That opens us up to shadowing problems.
Obviously changing the name of one or the other will need to happen.
Anyway, the idea is to show the general procedure for adding range
analysis to a domwalker based optimization/analysis pass.
Anyway, back to cleaning up vr_values :-)
Jeff
commit dca6f665879a75becb08697653a47ced71a627a0
Author: Jeff Law <l...@torsion.usersys.redhat.com>
Date: Thu Oct 26 14:19:42 2017 -0400
Range analyzer in sprintf code
diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 7415413..4ccc140 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -80,6 +80,7 @@ along with GCC; see the file COPYING3. If not see
#include "substring-locations.h"
#include "diagnostic.h"
#include "domwalk.h"
+#include "range-analyzer.h"
/* The likely worst case value of MB_LEN_MAX for the target, large enough
for UTF-8. Ideally, this would be obtained by a target hook if it were
@@ -121,12 +122,16 @@ class sprintf_dom_walker : public dom_walker
~sprintf_dom_walker () {}
virtual edge before_dom_children (basic_block) FINAL OVERRIDE;
+ virtual void after_dom_children (basic_block) FINAL OVERRIDE;
bool handle_gimple_call (gimple_stmt_iterator *);
struct call_info;
bool compute_format_length (call_info &, format_result *);
+ class range_analyzer range_analyzer;
};
+static class vr_values *x;
+
class pass_sprintf_length : public gimple_opt_pass
{
bool fold_return_value;
@@ -1143,7 +1148,7 @@ get_int_range (tree arg, HOST_WIDE_INT *pmin,
HOST_WIDE_INT *pmax,
{
/* Try to determine the range of values of the integer argument. */
wide_int min, max;
- enum value_range_type range_type = get_range_info (arg, &min, &max);
+ enum value_range_type range_type = x->get_range_info (arg, &min,
&max);
if (range_type == VR_RANGE)
{
HOST_WIDE_INT type_min
@@ -1443,7 +1448,7 @@ format_integer (const directive &dir, tree arg)
/* Try to determine the range of values of the integer argument
(range information is not available for pointers). */
wide_int min, max;
- enum value_range_type range_type = get_range_info (arg, &min, &max);
+ enum value_range_type range_type = x->get_range_info (arg, &min, &max);
if (range_type == VR_RANGE)
{
argmin = wide_int_to_tree (argtype, min);
@@ -3883,7 +3888,7 @@ sprintf_dom_walker::handle_gimple_call
(gimple_stmt_iterator *gsi)
of them at level 2. */
wide_int min, max;
enum value_range_type range_type
- = get_range_info (size, &min, &max);
+ = x->get_range_info (size, &min, &max);
if (range_type == VR_RANGE)
{
dstsize
@@ -3995,6 +4000,7 @@ sprintf_dom_walker::handle_gimple_call
(gimple_stmt_iterator *gsi)
edge
sprintf_dom_walker::before_dom_children (basic_block bb)
{
+ range_analyzer.enter (bb);
for (gimple_stmt_iterator si = gsi_start_bb (bb); !gsi_end_p (si); )
{
/* Iterate over statements, looking for function calls. */
@@ -4010,6 +4016,12 @@ sprintf_dom_walker::before_dom_children (basic_block bb)
return NULL;
}
+void
+sprintf_dom_walker::after_dom_children (basic_block bb)
+{
+ range_analyzer.leave (bb);
+}
+
/* Execute the pass for function FUN. */
unsigned int
@@ -4020,6 +4032,7 @@ pass_sprintf_length::execute (function *fun)
calculate_dominance_info (CDI_DOMINATORS);
sprintf_dom_walker sprintf_dom_walker;
+ x = &sprintf_dom_walker.range_analyzer.vr_values;
sprintf_dom_walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
/* Clean up object size info. */
