Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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 >
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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 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.o
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
On Thu, May 9, 2024 at 10:05 AM Mikael Morin 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
> > {
>
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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
Re: [PATCHv2] Value range: Add range op for __builtin_isfinite
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
{
