Re: [PATCH] Checking undefined_p before using the vr

[Jiufu Guo <guojiufu@linux.ibm.com>]

Hi,

Andrew MacLeod <amacleod@redhat.com> writes:

> On 9/12/23 21:42, Jiufu Guo wrote:
>> Hi,
>>
>> Richard Biener <rguenther@suse.de> writes:
>>
>>> On Thu, 7 Sep 2023, Jiufu Guo wrote:
>>>
>>>> Hi,
>>>>
>>>> As discussed in PR111303:
>>>>
>>>> For pattern "(X + C) / N": "div (plus@3 @0 INTEGER_CST@1) INTEGER_CST@2)",
>>>> Even if "X" has value-range and "X + C" does not overflow, "@3" may still
>>>> be undefined. Like below example:
>>>>
>>>> _3 = _2 + -5;
>>>> if (0 != 0)
>>>>    goto <bb 3>; [34.00%]
>>>> else
>>>>    goto <bb 4>; [66.00%]
>>>> ;;  succ:       3
>>>> ;;              4
>>>>
>>>> ;; basic block 3, loop depth 0
>>>> ;;  pred:       2
>>>> _5 = _3 / 5;
>>>> ;;  succ:       4
>>>>
>>>> The whole pattern "(_2 + -5 ) / 5" is in "bb 3", but "bb 3" would be
>>>> unreachable (because "if (0 != 0)" is always false).
>>>> And "get_range_query (cfun)->range_of_expr (vr3, @3)" is checked in
>>>> "bb 3", "range_of_expr" gets an "undefined vr3". Where "@3" is "_5".
>>>>
>>>> So, before using "vr3", it would be safe to check "!vr3.undefined_p ()".
>>>>
>>>> Bootstrap & regtest pass on ppc64{,le} and x86_64.
>>>> Is this ok for trunk?
>>> OK, but I wonder why ->range_of_expr () doesn't return false for
>>> undefined_p ()?  While "undefined" technically means we can treat
>>> it as nonnegative_p (or not, maybe but maybe not both), we seem to
>>> not want to do that.  So why expose it at all to ranger users
>>> (yes, internally we in some places want to handle undefined).
>> I guess, currently, it returns true and then lets the user check
>> undefined_p, maybe because it tries to only return false if the
>> type of EXPR is unsupported.
>
> false is returned if no range can be calculated for any reason. The
> most common ones are unsupported types or in some cases, statements
> that are not understood.  FALSE means you cannot use the range being
> passed in.

Thanks a lot for the explaination! "false" means no ranger returned:
we cannot use the range argument after call.

>
>
>> Let "range_of_expr" return false for undefined_p would save checking
>> undefined_p again when using the APIs.
>>
> undefined is a perfectly acceptable range.  It can be used to
> represent either values which has not been initialized, or more
> frequently it identifies values that cannot occur due to
> conflicting/unreachable code.  VARYING means it can be any range,
> UNDEFINED means this is unusable, so treat it accordingly.  Its
> propagated like any other range.

"undefined" means the ranger is unusable. So, for this ranger, it
seems only "undefined_p ()" can be checked, and it seems no other
functions of this ranger can be called.

I'm thinking that it may be ok to let "range_of_expr" return false
if the "vr" is "undefined_p".  I know this may change the meaning
of "range_of_expr" slightly :) 

>
> The only reason you are having issues is you are then asking for the
> type of the range, and an undefined range currently has no type, for
> historical reasons.

Yeap, thanks for pointing out this!

BR,
Jeff (Jiufu Guo)

>
> Andrew
>
> Andrew