| Message ID | 8a3f96a3-ce9b-48dc-b125-eaf1d832c79d@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] Value range: Add range op for __builtin_isfinite | expand |
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. 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. > + } > + 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 > {
Hi Mikael, Thanks for your comments. 在 2024/5/9 16:03, Mikael Morin 写道: > 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. > > 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. Should the varying be set by caller when fold_range returns false? Just like following codes in value-query.cc. if (!op.fold_range (r, type, r0, r1)) r.set_varying (type); Thanks Gui Haochen
Le 09/05/2024 à 10:47, HAO CHEN GUI a écrit : > Hi Mikael, > > Thanks for your comments. > > 在 2024/5/9 16:03, Mikael Morin 写道: >> 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. >> >> 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. > > Should the varying be set by caller when fold_range returns false? > Just like following codes in value-query.cc. > > if (!op.fold_range (r, type, r0, r1)) > r.set_varying (type); > Err, well, that's a good question. Looking more closely, there are at least two operators in range-op.cc that follow this pattern of returning false and letting the caller handle. And in gimple-range-op.cc many more of them, so your patch is canonical enough in any case, sorry for the false alarm. > Thanks > Gui Haochen
On Thu, May 9, 2024 at 10:05 AM Mikael Morin <morin-mikael@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. > > 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: 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: 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 > > { >
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-mikael@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
On 5/13/24 22:16, HAO CHEN GUI wrote: > Hi Aldy, > Thanks for your review comments. > > 在 2024/5/13 19:18, Aldy Hernandez 写道: >>>> +//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? In general, returning TRUE and VARYING is preferable for supported types. Returning FALSE usually means you are trying to fold a statement which produced a type which is unsupported. Or at least there is something in the statement we don't understand or want to deal with. The primary functional difference is that when we have a chain of operations (which is more common with op1_range and op2_range calls) , we will often continue processing and feed a VARYING result into the next operation. FALSE will usually terminate further processing. That said, we probably aren't super consistent because there are grey areas, but we probably should try to follow that model. In the end, you are unlikely to see much real difference between the 2 options. Andrew
On Tue, May 07, 2024 at 10:37:55AM +0800, HAO CHEN GUI wrote: > 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. BUILT_IN_ISFINITE is just one of many BUILT_IN_IS... builtins, would be nice to handle the others as well. E.g. isnormal/isnan/isinf, fpclassify etc. Note, the man page says for e.g. isnormal that it returns nonzero or zero, but in reality I think we implement it always inline and can check if it always returns [0,1]. Some others like isinf return [-1,1] though I think and fpclassify returns union of all the passed int values. Jakub
On 5/9/24 04:47, HAO CHEN GUI wrote: > Hi Mikael, > > Thanks for your comments. > > 在 2024/5/9 16:03, Mikael Morin 写道: >> 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. >> >> 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. > Should the varying be set by caller when fold_range returns false? > Just like following codes in value-query.cc. > > if (!op.fold_range (r, type, r0, r1)) > r.set_varying (type); > This would be dangerous in the general case. fold_range may have returned false because 'type' is an unsupported range type. Generally this is why we prefer range-ops to return TRUE and VARYING rather than FALSE for unknown values. When FALSE is returned, we should stop working with ranges because something is amok. Andrew
Hi Jakub, Thanks for your review comments. 在 2024/5/14 23:57, Jakub Jelinek 写道: > BUILT_IN_ISFINITE is just one of many BUILT_IN_IS... builtins, > would be nice to handle the others as well. > > E.g. isnormal/isnan/isinf, fpclassify etc. > Yes, I already sent the patches which add range op for isnormal/isnan/isinf for review. I will modify them according to review comments and submit them again. > Note, the man page says for e.g. isnormal that it returns nonzero or zero, > but in reality I think we implement it always inline and can check if > it always returns [0,1]. > Some others like isinf return [-1,1] though I think and fpclassify > returns union of all the passed int values. The gcc inline code always returns 0 or 1 for isnormal/isnan/isinf. But I wonder if all targets' expand can promise it. The rs6000 has an instruction for isnormal/isnan/isinf. So we're making the patch not to call inline codes and expand them by ourselves. Though rs6000 instruction returns 0 or 1 for them, not sure if other targets are the same. Thanks Gui Haochen
Hi Andrew, Thanks so much for your explanation. I got it. I will address the issue. Thanks Gui Haochen 在 2024/5/15 2:45, Andrew MacLeod 写道: > > On 5/9/24 04:47, HAO CHEN GUI wrote: >> Hi Mikael, >> >> Thanks for your comments. >> >> 在 2024/5/9 16:03, Mikael Morin 写道: >>> 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. >>> >>> 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. >> Should the varying be set by caller when fold_range returns false? >> Just like following codes in value-query.cc. >> >> if (!op.fold_range (r, type, r0, r1)) >> r.set_varying (type); >> > This would be dangerous in the general case. fold_range may have returned false because 'type' is an unsupported range type. Generally this is why we prefer range-ops to return TRUE and VARYING rather than FALSE for unknown values. When FALSE is returned, we should stop working with ranges because something is amok. > > Andrew >
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; + } + 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; + } +} op_cfn_isfinite; + // Implement range operator for CFN_BUILT_IN_ class cfn_parity : public range_operator { @@ -1288,6 +1338,11 @@ gimple_range_op_handler::maybe_builtin_call () m_operator = &op_cfn_isinf; break; + case CFN_BUILT_IN_ISFINITE: + m_op1 = gimple_call_arg (call, 0); + m_operator = &op_cfn_isfinite; + break; + CASE_CFN_COPYSIGN_ALL: m_op1 = gimple_call_arg (call, 0); m_op2 = gimple_call_arg (call, 1); diff --git a/gcc/testsuite/gcc.dg/tree-ssa/range-isfinite.c b/gcc/testsuite/gcc.dg/tree-ssa/range-isfinite.c new file mode 100644 index 00000000000..f5dce0a0486 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/range-isfinite.c @@ -0,0 +1,31 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-evrp" } */ + +#include <math.h> +void link_error(); + +void test1 (double x) +{ + if (x < __DBL_MAX__ && x > -__DBL_MAX__ && !__builtin_isfinite (x)) + link_error (); +} + +void test2 (float x) +{ + if (x < __FLT_MAX__ && x > -__FLT_MAX__ && !__builtin_isfinite (x)) + link_error (); +} + +void test3 (double x) +{ + if (__builtin_isfinite (x) && __builtin_isinf (x)) + link_error (); +} + +void test4 (float x) +{ + if (__builtin_isfinite (x) && __builtin_isinf (x)) + link_error (); +} + +/* { dg-final { scan-tree-dump-not "link_error" "evrp" } } */