[PATCH] Convert strlen pass from evrp to ranger.

[Richard Biener]

On Thu, Oct 21, 2021 at 2:56 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> On Thu, Oct 21, 2021 at 12:20 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Wed, Oct 20, 2021 at 10:58 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
> > >
> > >
> > >
> > > On 10/18/2021 2:17 AM, Aldy Hernandez wrote:
> > > >
> > > >
> > > > On 10/18/21 12:52 AM, Jeff Law wrote:
> > > >>
> > > >>
> > > >> On 10/8/2021 9:12 AM, Aldy Hernandez via Gcc-patches wrote:
> > > >>> The following patch converts the strlen pass from evrp to ranger,
> > > >>> leaving DOM as the last remaining user.
> > > >> So is there any reason why we can't convert DOM as well?   DOM's use
> > > >> of EVRP is pretty limited.  You've mentioned FP bits before, but my
> > > >> recollection is those are not part of the EVRP analysis DOM uses.
> > > >> Hell, give me a little guidance and I'll do the work...
> > > >
> > > > Not only will I take you up on that offer, but I can provide 90% of
> > > > the work.  Here be dragons, though (well, for me, maybe not for you ;-)).
> > > >
> > > > DOM is actually an evrp pass at -O1 in disguise.  The reason it really
> > > > is a covert evrp pass is because:
> > > >
> > > > a) It calls extract_range_from_stmt on each statement.
> > > >
> > > > b) It folds conditionals with simplify_using_ranges.
> > > >
> > > > c) But most importantly, it exports discovered ranges when it's done
> > > > (evrp_range_analyzer(true)).
> > > >
> > > > If you look at the evrp pass, you'll notice that that's basically what
> > > > it does, albeit with the substitute and fold engine, which also calls
> > > > gimple fold plus other goodies.
> > > >
> > > > But I could argue that we've made DOM into an evrp pass without
> > > > noticing.  The last item (c) is particularly invasive because these
> > > > exported ranges show up in other passes unexpectedly.  For instance, I
> > > > saw an RTL pass at -O1 miss an optimization because it was dependent
> > > > on some global range being set.  IMO, DOM should not export global
> > > > ranges it discovered during its walk (do one thing and do it well),
> > > > but I leave it to you experts to pontificate.
> > > All true.  But I don't think we've got many, if any, hard dependencies
> > > on those behaviors.
> > >
> > > >
> > > > The attached patch is rather trivial.  It's mostly deleting state.  It
> > > > seems DOM spends a lot of time massaging the IL so that it can fold
> > > > conditionals or thread paths.  None of this is needed, because the
> > > > ranger can do all of this.  Well, except floats, but...
> > > Massaging the IL should only take two forms IIRC.
> > >
> > > First, if we have a simplification we can do.  That could be const/copy
> > > propagation, replacing an expression with an SSA_NAME or constant and
> > > the like.  It doesn't massage the IL just to massage the IL.
> > >
> > > Second, we do temporarily copy propagate the current known values of an
> > > SSA name into use points and then see if that allows us to determine if
> > > a statement is already in the hash tables.  But we undo that so that
> > > nobody should see that temporary change in state.
> >
> > Are you sure we still do that?  I can't find it at least.
>
> I couldn't either, but perhaps what Jeff is referring to is the ad-hoc
> copy propagation that happens in the DOM's use of the threader:
>
>       /* Make a copy of the uses & vuses into USES_COPY, then cprop into
>          the operands.  */
>       FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
>         {
>           tree tmp = NULL;
>           tree use = USE_FROM_PTR (use_p);
>
>           copy[i++] = use;
>           if (TREE_CODE (use) == SSA_NAME)
>         tmp = SSA_NAME_VALUE (use);
>           if (tmp)
>         SET_USE (use_p, tmp);
>         }
>
>       cached_lhs = simplifier->simplify (stmt, stmt, bb, this);
>
>       /* Restore the statement's original uses/defs.  */
>       i = 0;
>       FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
>         SET_USE (use_p, copy[i++]);

Ah, likely.  These days we'd likely use a gimple_match_op but then
this seems heavily abstracted, no idea where simplifier->simplify
might lead to ;)
I'm also not sure why the threader would do the valueization here and
not the simplify() function (and lookup_avail_expr misses an 'exploded' operand
lookup as well).  Lot's of legacy code ;)

But I think the above is not going to be an issue unless ranger runs in
circles around backedges, arriving at this very same stmt again?  A way
out might be to copy the stmt to the stack, adjust operands and use that
for the simplify entry.

Richard.

> Aldy
>