[PATCHv2] Value range: Add range op for __builtin_isfinite

  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
  

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
>