On 11/15/22 08:15, Richard Biener wrote: > On Mon, Nov 14, 2022 at 8:05 PM Aldy Hernandez wrote: >> >> >> >> On 11/14/22 10:12, Richard Biener wrote: >>> On Sat, Nov 12, 2022 at 7:30 PM Aldy Hernandez 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