On 11/15/22 08:15, Richard Biener wrote:
On Mon, Nov 14, 2022 at 8:05 PM Aldy Hernandez <al...@redhat.com> wrote:
On 11/14/22 10:12, Richard Biener wrote:
On Sat, Nov 12, 2022 at 7:30 PM Aldy Hernandez <al...@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
Woah, sorry. That was not intended. For that matter, the patch as
posted caused:
FAIL: gcc.dg/builtins-10.c (test for excess errors)
FAIL: gcc.dg/builtins-57.c (test for excess errors)
FAIL: gcc.dg/torture/builtin-nonneg-1.c -O1 (test for excess errors)
FAIL: gcc.dg/torture/builtin-nonneg-1.c -O2 (test for excess errors)
FAIL: gcc.dg/torture/builtin-nonneg-1.c -O2 -flto
-fno-use-linker-plugin -flto-partition=none (test for excess errors)
FAIL: gcc.dg/torture/builtin-nonneg-1.c -O3 -g (test for excess errors)
FAIL: gcc.dg/torture/builtin-nonneg-1.c -Os (test for excess errors)
FAIL: gcc.dg/torture/builtin-power-1.c -O1 (test for excess errors)
FAIL: gcc.dg/torture/builtin-power-1.c -O2 (test for excess errors)
FAIL: gcc.dg/torture/builtin-power-1.c -O2 -flto
-fno-use-linker-plugin -flto-partition=none (test for excess errors)
FAIL: gcc.dg/torture/builtin-power-1.c -O3 -g (test for excess errors)
FAIL: gcc.dg/torture/builtin-power-1.c -Os (test for excess errors)
Note that ranger folding calls this function, though it won't run the
risk of endless recursion because range_of_stmt uses the LHS, and only
use global ranges to solve the LHS.
Also, frange::supports_p() does not support all floats:
static bool supports_p (const_tree type)
{
// ?? Decimal floats can have multiple representations for the
// same number. Supporting them may be as simple as just
// disabling them in singleton_p. No clue.
return SCALAR_FLOAT_TYPE_P (type) && !DECIMAL_FLOAT_TYPE_P (type);
}
Finally, my patch is more conservative than what the *nonnegative_warn*
friends do. We only return true when we're sure about the sign bit and
it's FALSE. As I mentioned elsewhere, tree_call_nonnegative_warn_p()
always returns true for:
CASE_CFN_ACOS:
CASE_CFN_ACOS_FN:
CASE_CFN_ACOSH:
CASE_CFN_ACOSH_FN:
CASE_CFN_CABS:
CASE_CFN_CABS_FN:
...
...
/* Always true. */
return true;
This means that we'll return true for a NAN, but we're incorrectly
assuming the NAN will be +NAN. In my proposed patch, we don't make such
assumptions. We only return true if the range is non-negative,
including the NAN.
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 ;)
Updated patch that passes test.
OK? And if so, can I close the PR?
Thanks.
Aldy
From 8f589cff22e80eda7cc3837080dbc9ae8280da8c Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <al...@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..c4ebe9a3b52 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;
+ if (get_global_range_query ()->range_of_stmt (r, stmt)
+ && r.signbit_p (sign))
+ return !sign;
+ }
switch (gimple_code (stmt))
{
case GIMPLE_ASSIGN:
--
2.38.1
