get_range_query vs NULL argument

get_range_query vs NULL argument

[Andrew Pinski]

Hi,
  While fixing PR 108354, I came across that
ssa_name_has_boolean_range calls get_range_query with cfun as the
argument but sometimes while in IPA passes cfun is currently nullptr.
The question should we check the argument before calling
get_range_query or is it a valid thing to call it with a nullptr (and
have it return global_ranges in that case)?

I committed the patch (for PR 108354) with the workaround around the
issue via having the match.pd pattern which was calling
ssa_name_has_boolean_range only be invoked from the gimple
simplifiers.

Below is the patch which undones the workaround:
```
diff --git a/gcc/match.pd b/gcc/match.pd
index e7b700349a6..e352bd422f5 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -1732,7 +1732,6 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
  (if (!FIXED_POINT_TYPE_P (type))
  (plus @0 (negate @1))))

-#if GIMPLE
 /* 1 - a is a ^ 1 if a had a bool range. */
 /* This is only enabled for gimple as sometimes
    cfun is not set for the function which contains
@@ -1743,7 +1742,6 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (if (INTEGRAL_TYPE_P (type)
        && ssa_name_has_boolean_range (@1))
    (bit_xor @1 @0)))
-#endif

 /* Other simplifications of negation (c.f. fold_negate_expr_1).  */
 (simplify
```
Note I can only so far reproduce the call to
ssa_name_has_boolean_range that causes an issue while building Ada
tools (while bootstrapping) because the code that needs to hit this is
related to variable sized type accesses.

Thanks,
Andrew Pinski

Re: get_range_query vs NULL argument

[Andrew MacLeod]

On 2/15/23 14:50, Andrew Pinski wrote:
> Hi,
>    While fixing PR 108354, I came across that
> ssa_name_has_boolean_range calls get_range_query with cfun as the
> argument but sometimes while in IPA passes cfun is currently nullptr.
> The question should we check the argument before calling
> get_range_query or is it a valid thing to call it with a nullptr (and
> have it return global_ranges in that case)?

That might be ok...  personally I see nothing wrong with:

diff --git a/gcc/value-query.h b/gcc/value-query.h
index 63878968118..2d7bf8fcf33 100644
--- a/gcc/value-query.h
+++ b/gcc/value-query.h
@@ -140,7 +140,7 @@ get_global_range_query ()
  ATTRIBUTE_RETURNS_NONNULL inline range_query *
  get_range_query (const struct function *fun)
  {
-  return fun->x_range_query ? fun->x_range_query : &global_ranges;
+  return (fun && fun->x_range_query) ? fun->x_range_query : &global_ranges;
  }

  // Query the global range of NAME in function F.  Default to cfun.


The client is probably going to do that anyway.

Aldy?

Re: get_range_query vs NULL argument

[Richard Biener]

On Wed, Feb 15, 2023 at 11:31 PM Andrew MacLeod via Gcc <gcc@gcc.gnu.org> wrote:
>
>
> On 2/15/23 14:50, Andrew Pinski wrote:
> > Hi,
> >    While fixing PR 108354, I came across that
> > ssa_name_has_boolean_range calls get_range_query with cfun as the
> > argument but sometimes while in IPA passes cfun is currently nullptr.
> > The question should we check the argument before calling
> > get_range_query or is it a valid thing to call it with a nullptr (and
> > have it return global_ranges in that case)?
>
> That might be ok...  personally I see nothing wrong with:
>
> diff --git a/gcc/value-query.h b/gcc/value-query.h
> index 63878968118..2d7bf8fcf33 100644
> --- a/gcc/value-query.h
> +++ b/gcc/value-query.h
> @@ -140,7 +140,7 @@ get_global_range_query ()
>   ATTRIBUTE_RETURNS_NONNULL inline range_query *
>   get_range_query (const struct function *fun)
>   {
> -  return fun->x_range_query ? fun->x_range_query : &global_ranges;
> +  return (fun && fun->x_range_query) ? fun->x_range_query : &global_ranges;
>   }
>
>   // Query the global range of NAME in function F.  Default to cfun.
>
>
> The client is probably going to do that anyway.

But if there's no 'fun', what is 'global_ranges' initialized for?  Or
is 'global_ranges'
usable in IPA context?

> Aldy?
>
>

Re: get_range_query vs NULL argument

[Aldy Hernandez]

On 2/16/23 08:57, Richard Biener wrote:
> On Wed, Feb 15, 2023 at 11:31 PM Andrew MacLeod via Gcc <gcc@gcc.gnu.org> wrote:
>>
>>
>> On 2/15/23 14:50, Andrew Pinski wrote:
>>> Hi,
>>>     While fixing PR 108354, I came across that
>>> ssa_name_has_boolean_range calls get_range_query with cfun as the
>>> argument but sometimes while in IPA passes cfun is currently nullptr.
>>> The question should we check the argument before calling
>>> get_range_query or is it a valid thing to call it with a nullptr (and
>>> have it return global_ranges in that case)?
>>
>> That might be ok...  personally I see nothing wrong with:
>>
>> diff --git a/gcc/value-query.h b/gcc/value-query.h
>> index 63878968118..2d7bf8fcf33 100644
>> --- a/gcc/value-query.h
>> +++ b/gcc/value-query.h
>> @@ -140,7 +140,7 @@ get_global_range_query ()
>>    ATTRIBUTE_RETURNS_NONNULL inline range_query *
>>    get_range_query (const struct function *fun)
>>    {
>> -  return fun->x_range_query ? fun->x_range_query : &global_ranges;
>> +  return (fun && fun->x_range_query) ? fun->x_range_query : &global_ranges;
>>    }
>>
>>    // Query the global range of NAME in function F.  Default to cfun.
>>
>>
>> The client is probably going to do that anyway.
> 
> But if there's no 'fun', what is 'global_ranges' initialized for?  Or
> is 'global_ranges'
> usable in IPA context?

If there is no fun, global_ranges will just return what get_range_info() 
used to return (i.e. SSA_NAME_RANGE_INFO).

Aldy

Re: get_range_query vs NULL argument

[Andrew Pinski]

On Wed, Feb 15, 2023 at 2:30 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>
>
> On 2/15/23 14:50, Andrew Pinski wrote:
> > Hi,
> >    While fixing PR 108354, I came across that
> > ssa_name_has_boolean_range calls get_range_query with cfun as the
> > argument but sometimes while in IPA passes cfun is currently nullptr.
> > The question should we check the argument before calling
> > get_range_query or is it a valid thing to call it with a nullptr (and
> > have it return global_ranges in that case)?
>
> That might be ok...  personally I see nothing wrong with:
>
> diff --git a/gcc/value-query.h b/gcc/value-query.h
> index 63878968118..2d7bf8fcf33 100644
> --- a/gcc/value-query.h
> +++ b/gcc/value-query.h
> @@ -140,7 +140,7 @@ get_global_range_query ()
>   ATTRIBUTE_RETURNS_NONNULL inline range_query *
>   get_range_query (const struct function *fun)
>   {
> -  return fun->x_range_query ? fun->x_range_query : &global_ranges;
> +  return (fun && fun->x_range_query) ? fun->x_range_query : &global_ranges;
>   }
>
>   // Query the global range of NAME in function F.  Default to cfun.
>
>
> The client is probably going to do that anyway.

Ok. I will submit a patch which does the above; I had that patch
already when I wrote the email and wanted to double check before
submitting it.

Thanks,
Andrew Pinski

>
> Aldy?
>
>