Hi Aldy, Thanks for your review comments. 在 2024/5/13 19:18, Aldy Hernandez 写道: > On Thu, May 9, 2024 at 10:05 AM Mikael Morin <morin-mik...@orange.fr> wrote: >> >> Hello, >> >> Le 07/05/2024 à 04:37, HAO CHEN GUI a écrit : >>> Hi, >>> The former patch adds isfinite optab for __builtin_isfinite. >>> https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649339.html >>> >>> Thus the builtin might not be folded at front end. The range op for >>> isfinite is needed for value range analysis. This patch adds them. >>> >>> Compared to last version, this version fixes a typo. >>> >>> Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no >>> regressions. Is it OK for the trunk? >>> >>> Thanks >>> Gui Haochen >>> >>> ChangeLog >>> Value Range: Add range op for builtin isfinite >>> >>> The former patch adds optab for builtin isfinite. Thus builtin isfinite >>> might >>> not be folded at front end. So the range op for isfinite is needed for >>> value >>> range analysis. This patch adds range op for builtin isfinite. >>> >>> gcc/ >>> * gimple-range-op.cc (class cfn_isfinite): New. >>> (op_cfn_finite): New variables. >>> (gimple_range_op_handler::maybe_builtin_call): Handle >>> CFN_BUILT_IN_ISFINITE. >>> >>> gcc/testsuite/ >>> * gcc/testsuite/gcc.dg/tree-ssa/range-isfinite.c: New test. >>> >>> patch.diff >>> diff --git a/gcc/gimple-range-op.cc b/gcc/gimple-range-op.cc >>> index 9de130b4022..99c511728d3 100644 >>> --- a/gcc/gimple-range-op.cc >>> +++ b/gcc/gimple-range-op.cc >>> @@ -1192,6 +1192,56 @@ public: >>> } >>> } op_cfn_isinf; >>> >>> +//Implement range operator for CFN_BUILT_IN_ISFINITE >>> +class cfn_isfinite : public range_operator >>> +{ >>> +public: >>> + using range_operator::fold_range; >>> + using range_operator::op1_range; >>> + virtual bool fold_range (irange &r, tree type, const frange &op1, >>> + const irange &, relation_trio) const override >>> + { >>> + if (op1.undefined_p ()) >>> + return false; >>> + >>> + if (op1.known_isfinite ()) >>> + { >>> + r.set_nonzero (type); >>> + return true; >>> + } >>> + >>> + if (op1.known_isnan () >>> + || op1.known_isinf ()) >>> + { >>> + r.set_zero (type); >>> + return true; >>> + } >>> + >>> + return false; >> I think the canonical API behaviour sets R to varying and returns true >> instead of just returning false if nothing is known about the range. > > Correct. If we know it's varying, we just set varying and return > true. Returning false is usually reserved for "I have no idea". > However, every caller of fold_range() should know to ignore a return > of false, so you should be safe.
So it's better to set varying here and return true? > >> >> I'm not sure whether it makes any difference; Aldy can probably tell. >> But if the type is bool, varying is [0,1] which is better than unknown >> range. > > Also, I see you're setting zero/nonzero. Is the return type known to > be boolean, because if so, we usually prefer to one of: The return type is int. For __builtin_isfinite, the result is nonzero when the float is a finite number, 0 otherwise. > > r = range_true () > r = range_false () > r = range_true_and_false (); > > It doesn't matter either way, but it's probably best to use these as > they force boolean_type_node automatically. > > I don't have a problem with this patch, but I would prefer the > floating point savvy people to review this, as there are no members of > the ranger team that are floating point experts :). > > Also, I see you mention in your original post that this patch was > needed as a follow-up to this one: > > https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649339.html > > I don't see the above patch in the source tree currently: Sorry, I may not express it clear. I sent a series of patches for review. Some patches depend on others. The patch I mentioned is a patch also under review. Here is the list of the series of patches. Some of them are generic, and others are rs6000 specific. [PATCH] Value Range: Add range op for builtin isinf https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648303.html [patch, rs6000] Implement optab_isinf for SFmode, DFmode and TFmode [PR97786] https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648304.html [Patch] Builtin: Fold builtin_isinf on IBM long double to builtin_isinf on double [PR97786] https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648433.html [PATCH] Optab: add isfinite_optab for __builtin_isfinite https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649339.html [PATCHv2] Value range: Add range op for __builtin_isfinite https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650857.html [PATCH-2, rs6000] Implement optab_isfinite for SFmode, DFmode and TFmode [PR97786] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649346.html [PATCH-3] Builtin: Fold builtin_isfinite on IBM long double to builtin_isfinite on double [PR97786] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649351.html [PATCH] Optab: add isnormal_optab for __builtin_isnormal https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649366.html [PATCH-4, rs6000] Implement optab_isnormal for SFmode, DFmode and TFmode [PR97786] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649368.html > > Thanks. > Aldy > >> >>> + } >>> + virtual bool op1_range (frange &r, tree type, const irange &lhs, >>> + const frange &, relation_trio) const override >>> + { >>> + if (lhs.zero_p ()) >>> + { >>> + // The range is [-INF,-INF][+INF,+INF] NAN, but it can't be >>> represented. >>> + // Set range to varying >>> + r.set_varying (type); >>> + return true; >>> + } >>> + >>> + if (!range_includes_zero_p (&lhs)) >>> + { >>> + nan_state nan (false); >>> + r.set (type, real_min_representable (type), >>> + real_max_representable (type), nan); >>> + return true; >>> + } >>> + >>> + return false; >> Same here. >> >>> + } >>> +} op_cfn_isfinite; >>> + >>> // Implement range operator for CFN_BUILT_IN_ >>> class cfn_parity : public range_operator >>> { >> > Thanks Gui Haochen