Re: [PATCH] [PR68097] Try to avoid recursing for floats in tree_*_nonnegative_warnv_p.

[Richard Biener <richard.guenther@gmail.com>]

On Mon, Nov 14, 2022 at 8:05 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 11/14/22 10:12, Richard Biener wrote:
> > On Sat, Nov 12, 2022 at 7:30 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >> It irks me that a PR named "we should track ranges for floating-point
> >> hasn't been closed in this release.  This is an attempt to do just
> >> that.
> >>
> >> As mentioned in the PR, even though we track ranges for floats, it has
> >> been suggested that avoiding recursing through SSA defs in
> >> gimple_assign_nonnegative_warnv_p is also a goal.  We can do this with
> >> various ranger components without the need for a heavy handed approach
> >> (i.e. a full ranger).
> >>
> >> I have implemented two versions of known_float_sign_p() that answer
> >> the question whether we definitely know the sign for an operation or a
> >> tree expression.
> >>
> >> Both versions use get_global_range_query, which is a wrapper to query
> >> global ranges.  This means, that no caching or propagation is done.
> >> In the case of an SSA, we just return the global range for it (think
> >> SSA_NAME_RANGE_INFO).  In the case of a tree code with operands, we
> >> also use get_global_range_query to resolve the operands, and then call
> >> into range-ops, which is our lowest level component.  There is no
> >> ranger or gori involved.  All we're doing is resolving the operation
> >> with the ranges passed.
> >>
> >> This is enough to avoid recursing in the case where we definitely know
> >> the sign of a range.  Otherwise, we still recurse.
> >>
> >> Note that instead of get_global_range_query(), we could use
> >> get_range_query() which uses a ranger (if active in a pass), or
> >> get_global_range_query if not.  This would allow passes that have an
> >> active ranger (with enable_ranger) to use a full ranger.  These passes
> >> are currently, VRP, loop unswitching, DOM, loop versioning, etc.  If
> >> no ranger is active, get_range_query defaults to global ranges, so
> >> there's no additional penalty.
> >>
> >> Would this be acceptable, at least enough to close (or rename the PR ;-))?
> >
> > I think the checks would belong to the gimple_stmt_nonnegative_warnv_p function
> > only (that's the SSA name entry from the fold-const.cc ones)?
>
> That was my first approach, but I thought I'd cover the unary and binary
> operators as well, since they had other callers.  But I'm happy with
> just the top-level tweak.  It's a lot less code :).

@@ -9234,6 +9235,15 @@ bool
 gimple_stmt_nonnegative_warnv_p (gimple *stmt, bool *strict_overflow_p,
                                 int depth)
 {
+  tree type = gimple_range_type (stmt);
+  if (type && frange::supports_p (type))
+    {
+      frange r;
+      bool sign;
+      return (get_global_range_query ()->range_of_stmt (r, stmt)
+             && r.signbit_p (sign)
+             && sign == false);
+    }

the above means we never fall through to the switch below if
frange::supports_p (type) - that's eventually good enough, I
don't think we ever call this very function directly but it gets
invoked via recursion through operands only.  But of course
I wonder what types are not supported by frange and whether
the manual processing we fall through to does anything meaningful
for those?

I won't ask you to thoroughly answer this now but please put in
a comment reflecting the above before the switch stmt.

   switch (gimple_code (stmt))


Otherwise OK, in case you tree gets back to bootstrapping ;)

> >
> > I also notice the use of 'bool' for the "sign".  That's not really
> > descriptive.  We
> > have SIGNED and UNSIGNED (aka enum signop), not sure if that's the
> > perfect match vs. NEGATIVE and NONNEGATIVE.  Maybe the functions
> > name is just bad and they should be known_float_negative_p?
>
> The bool sign is to keep in line with real.*, and was suggested by Jeff
> (in real.* not here).  I'm happy to change the entire frange API to use
> sgnop.  It is cleaner.  If that's acceptable, I could do that as a
> follow-up.
>
> How's this, pending tests once I figure out why my trees have been
> broken all day :-/.
>
> Aldy
>
> p.s. First it was sphinx failure, now I'm seeing this:
> /home/aldyh/src/clean/gcc/match.pd:7935:8 error: return statement not
> allowed in C expression
>         return NULL_TREE;
>         ^

Supposedly somebody pushed and reverted this transient error?  Yep,
Tamar did.

Richard.