[PATCH] generalized range_query class for multiple contexts

[Martin Sebor]

On 10/1/20 9:34 AM, Aldy Hernandez wrote:
> 
> 
> 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.

Ack.  I'll be on the lookout for the ranger commit (if you hppen
to remember and CC me on it just in case I might miss it that would
be great).

Thanks
Martin

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