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

[Aldy Hernandez <aldyh@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3095 bytes --]



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 :).

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

[-- Attachment #2: 0001-PR68097-Try-to-avoid-recursing-for-floats-in-gimple_.patch --]
[-- Type: text/x-patch, Size: 1794 bytes --]

From 6e36626aec81bf97f8f54116a291574c16cbc205 Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <aldyh@redhat.com>
Date: Sat, 12 Nov 2022 11:58:07 +0100
Subject: [PATCH] [PR68097] Try to avoid recursing for floats in
 gimple_stmt_nonnegative_warnv_p.

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.  This patch uses a
global range query (no on-demand lookups, just global ranges and
minimal folding) to determine if the range of a statement is known to
be non-negative.

	PR tree-optimization/68097

gcc/ChangeLog:

	* gimple-fold.cc (gimple_stmt_nonnegative_warnv_p): Call
	range_of_stmt for floats.
---
 gcc/gimple-fold.cc | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
index 0a212e6d0d4..79cc4d7f569 100644
--- a/gcc/gimple-fold.cc
+++ b/gcc/gimple-fold.cc
@@ -68,6 +68,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-strlen.h"
 #include "varasm.h"
 #include "internal-fn.h"
+#include "gimple-range.h"
 
 enum strlen_range_kind {
   /* Compute the exact constant string length.  */
@@ -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);
+    }
   switch (gimple_code (stmt))
     {
     case GIMPLE_ASSIGN:
-- 
2.38.1