[PATCH] generalized range_query class for multiple contexts

[Aldy Hernandez]

On 10/1/20 3:22 PM, Andrew MacLeod wrote:
 > On 10/1/20 5:05 AM, Aldy Hernandez via Gcc-patches wrote:
 >>> Thanks for doing all this!  There isn't anything I don't understand
 >>> in the sprintf changes so no questions from me (well, almost none).
 >>> Just some comments:
 >> Thanks for your comments on the sprintf/strlen API conversion.
 >>
 >>> The current call statement is available in all functions that take
 >>> a directive argument, as dir->info.callstmt.  There should be no need
 >>> to also add it as a new argument to the functions that now need it.
 >> Fixed.
 >>
 >>> The change adds code along these lines in a bunch of places:
 >>>
 >>> +         value_range vr;
 >>> +         if (!query->range_of_expr (vr, arg, stmt))
 >>> +           vr.set_varying (TREE_TYPE (arg));
 >>>
 >>> I thought under the new Ranger APIs when a range couldn't be
 >>> determined it would be automatically set to the maximum for
 >>> the type.  I like that and have been moving in that direction
 >>> with my code myself (rather than having an API fail, have it
 >>> set the max range and succeed).
 >> I went through all the above idioms and noticed all are being used on
 >> supported types (integers or pointers).  So range_of_expr will always
 >> return true.  I've removed the if() and the set_varying.
 >>
 >>> Since that isn't so in this case, I think it would still be nice
 >>> if the added code could be written as if the range were set to
 >>> varying in this case and (ideally) reduced to just initialization:
 >>>
 >>>     value_range vr = some-function (query, stmt, arg);
 >>>
 >>> some-function could be an inline helper defined just for the sprintf
 >>> pass (and maybe also strlen which also seems to use the same pattern),
 >>> or it could be a value_range AKA irange ctor, or it could be a member
 >>> of range_query, whatever is the most appropriate.
 >>>
 >>> (If assigning/copying a value_range is thought to be too expensive,
 >>> declaring it first and then passing it to that helper to set it
 >>> would work too).
 >>>
 >>> In strlen, is the removed comment no longer relevant?  (I.e., does
 >>> the ranger solve the problem?)
 >>>
 >>> -      /* The range below may be "inaccurate" if a constant has been
 >>> -        substituted earlier for VAL by this pass that hasn't been
 >>> -        propagated through the CFG.  This shoud be fixed by the new
 >>> -        on-demand VRP if/when it becomes available (hopefully in
 >>> -        GCC 11).  */
 >> It should.
 >>
 >>> I'm wondering about the comment added to get_range_strlen_dynamic
 >>> and other places:
 >>>
 >>> +         // FIXME: Use range_query instead of global ranges.
 >>>
 >>> Is that something you're planning to do in a followup or should
 >>> I remember to do it at some point?
 >> I'm not planning on doing it.  It's just a reminder that it would be
 >> beneficial to do so.
 >>
 >>> Otherwise I have no concern with the changes.
 >> It's not cleared whether Andrew approved all 3 parts of the patchset
 >> or just the valuation part.  I'll wait for his nod before committing
 >> this chunk.
 >>
 >> Aldy
 >>
 > I have no issue with it, so OK.

Pushed all 3 patches.

 >
 > Just an observation that should be pointed out, I believe Aldy has all
 > the code for converting to a ranger, but we have not pursued that any
 > further yet since there is a regression due to our lack of equivalence
 > processing I think?  That should be resolved in the coming month, but at
 > the moment is a holdback/concern for converting these passes...  iirc.

Yes.  Martin, the take away here is that the strlen/sprintf pass has 
been converted to the new API, but ranger is still not up and running on 
it (even on the branch).

With the new API, all you have to do is remove all instances of 
evrp_range_analyzer and replace them with a ranger.  That's it.
Below is an untested patch that would convert you to a ranger once it's 
contributed.

IIRC when I enabled the ranger for your pass a while back, there was one 
or two regressions due to missing equivalences, and the rest were 
because the tests were expecting an actual specific range, and the 
ranger returned a slightly different/better one.  You'll need to adjust 
your tests.

Aldy

diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index f4d1c5ca256..9f9e95b7155 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -58,8 +58,8 @@ along with GCC; see the file COPYING3.  If not see
  #include "tree-ssa-loop.h"
  #include "tree-scalar-evolution.h"
  #include "vr-values.h"
-#include "gimple-ssa-evrp-analyze.h"
  #include "tree-ssa.h"
+#include "gimple-range.h"

  /* A vector indexed by SSA_NAME_VERSION.  0 means unknown, positive value
     is an index into strinfo vector, negative value stands for
@@ -5755,16 +5755,13 @@ class strlen_dom_walker : public dom_walker
  public:
    strlen_dom_walker (cdi_direction direction)
      : dom_walker (direction),
-    evrp (false),
      m_cleanup_cfg (false)
    {}

    virtual edge before_dom_children (basic_block);
    virtual void after_dom_children (basic_block);

-  /* EVRP analyzer used for printf argument range processing, and
-     to track strlen results across integer variable assignments.  */
-  evrp_range_analyzer evrp;
+  gimple_ranger ranger;

    /* Flag that will trigger TODO_cleanup_cfg to be returned in strlen
       execute function.  */
@@ -5777,8 +5774,6 @@ public:
  edge
  strlen_dom_walker::before_dom_children (basic_block bb)
  {
-  evrp.enter (bb);
-
    basic_block dombb = get_immediate_dominator (CDI_DOMINATORS, bb);

    if (dombb == NULL)
@@ -5853,13 +5848,7 @@ strlen_dom_walker::before_dom_children 
(basic_block bb)
    /* Attempt to optimize individual statements.  */
    for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
      {
-      gimple *stmt = gsi_stmt (gsi);
-
-      /* First record ranges generated by this statement so they
-	 can be used by printf argument processing.  */
-      evrp.record_ranges_from_stmt (stmt, false);
-
-      if (check_and_optimize_stmt (&gsi, &cleanup_eh, &evrp))
+      if (check_and_optimize_stmt (&gsi, &cleanup_eh, &ranger))
  	gsi_next (&gsi);
      }

@@ -5878,8 +5867,6 @@ strlen_dom_walker::before_dom_children 
(basic_block bb)
  void
  strlen_dom_walker::after_dom_children (basic_block bb)
  {
-  evrp.leave (bb);
-
    if (bb->aux)
      {
        stridx_to_strinfo = ((vec<strinfo *, va_heap, vl_embed> *) bb->aux);