Hi,
On Fri, Nov 15 2024, Martin Jambor wrote:
> Hi,
>
> On Thu, Nov 07 2024, Aldy Hernandez wrote:
>> Jan Hubicka <hubi...@ucw.cz> writes:
>>
>>>> > 2024-11-01 Martin Jambor <mjam...@suse.cz>
>>>> >
>>>> > * ipa-prop.cc (ipa_compute_jump_functions_for_edge): When
>>>> > creating
>>>> > value-range jump functions from pointer type constant zero, do so
>>>> > as if it was not a pointer.
>>>> > ---
>>>> > gcc/ipa-prop.cc | 3 ++-
>>>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>>>> >
>>>> > diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
>>>> > index 9bd2e4bc60c..012f8a32386 100644
>>>> > --- a/gcc/ipa-prop.cc
>>>> > +++ b/gcc/ipa-prop.cc
>>>> > @@ -2368,7 +2368,8 @@ ipa_compute_jump_functions_for_edge (struct
>>>> > ipa_func_body_info *fbi,
>>>> > }
>>>> >
>>>> > value_range vr (TREE_TYPE (arg));
>>>> > - if (POINTER_TYPE_P (TREE_TYPE (arg)))
>>>> > + if (POINTER_TYPE_P (TREE_TYPE (arg))
>>>> > + && !zerop (arg))
>>>>
>>>> integer_zerop (arg) - I also think this deserves a comment.
>
> thanks for the pointer, I was not aware of that function. But given
> Honza's and Aldi's feedback, I may take a different path.
>
>>>
>>> Comment would indeed be nice. It is not clear to me why special
>>> handling is needed here and ranger does not give the same or better
>>> value range than one we compute based on alignment+offset and non-zero
>>> ness?
>>
>> Yeah, this doesn't smell right. Martin, could you look at what's going on?
>
> If you quickly glance at the code it is not surprising. The pointer
> handling code looks if it knows that the argument is non-zero, and
> depending on that either starts with a (freshly initialized) non_zero or
> varying value_range. Afterwards, it proceeds to attempt to imbue it
> with alignment info. That is, the code does not try to store the result
> of the get_range_query into the jump function, it is simply interested
> in the non-NULLness.
>
> I thought that was intentional but given Aldy's reaction perhaps it
> wasn't, so I decided to be bolder and rework the code a bit. Please see
> an alternative patch below.
>
>>
>>>
>>> The code was needed since we did not have value ranges for pointer typed
>>> SSA names, but do we still need to special case them these days?
>>
>> Note that the prange implementation doesn't do anything extra we weren't
>> already doing with irange for pointers. And the original code didn't
>> update ranges or value/mask pairs based on alignment, so you probably
>> still have to keep doing whatever alignment magic you were doing.
>
> I also work with the assumption that the extra code is necessary but my
> understanding of ranger and its capabilities is limited.
>
> Below is the new patch which has also passed bootstrap and testing on
> x86_64-linux on its own and along with the verifier patch.
>
Ping, please.
Martin
> ---------- 8< -------------------- 8< -------------------- 8< ----------
>
> When looking into cases where we know an actual argument of a call is
> a constant but we don't generate a singleton value-range for the jump
> function, I found out that the special handling of pointer constants
> does not work well for constant zero pointer values. In fact the code
> only attempts to see if it can figure out that an argument is not zero
> and if it can figure out any alignment information.
>
> With this patch, we try to use the value_range that ranger can give us
> in the jump function if we can and we query ranger for all kinds of
> arguments, not just SSA_NAMES (and so also pointer integer constants).
> If we cannot figure out a useful range we fall back again on figuring
> out non-NULLness with tree_single_nonzero_warnv_p.
>
> With this patch, we generate
>
> [prange] struct S * [0, 0] MASK 0x0 VALUE 0x0
>
> instead of for example:
>
> [prange] struct S * [0, +INF] MASK 0xfffffffffffffff0 VALUE 0x0
>
> for a zero constant passed in a call.
>
> If you are wondering why we check whether the value range obtained
> from range_of_expr can be undefined, even when the function returns
> true, that is because that can apparently happen for default-definition
> SSA_NAMEs.
>
> gcc/ChangeLog:
>
> 2024-11-15 Martin Jambor <mjam...@suse.cz>
>
> * ipa-prop.cc (ipa_compute_jump_functions_for_edge): Try harder to
> use the value range obtained from ranger for pointer values.
> ---
> gcc/ipa-prop.cc | 35 ++++++++++++++++-------------------
> 1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index fd0d9b7c15c..50ec8e0cf28 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -2382,28 +2382,27 @@ ipa_compute_jump_functions_for_edge (struct
> ipa_func_body_info *fbi,
> value_range vr (TREE_TYPE (arg));
> if (POINTER_TYPE_P (TREE_TYPE (arg)))
> {
> - bool addr_nonzero = false;
> - bool strict_overflow = false;
> -
> - if (TREE_CODE (arg) == SSA_NAME
> - && param_type
> - && get_range_query (cfun)->range_of_expr (vr, arg, cs->call_stmt)
> - && vr.nonzero_p ())
> - addr_nonzero = true;
> - else if (tree_single_nonzero_warnv_p (arg, &strict_overflow))
> - addr_nonzero = true;
> -
> - if (addr_nonzero)
> - vr.set_nonzero (TREE_TYPE (arg));
> -
> + if (!get_range_query (cfun)->range_of_expr (vr, arg, cs->call_stmt)
> + || vr.varying_p ()
> + || vr.undefined_p ())
> + {
> + bool strict_overflow = false;
> + if (tree_single_nonzero_warnv_p (arg, &strict_overflow))
> + vr.set_nonzero (TREE_TYPE (arg));
> + else
> + vr.set_varying (TREE_TYPE (arg));
> + }
> + gcc_assert (!vr.undefined_p ());
> unsigned HOST_WIDE_INT bitpos;
> - unsigned align, prec = TYPE_PRECISION (TREE_TYPE (arg));
> + unsigned align = BITS_PER_UNIT;
>
> - get_pointer_alignment_1 (arg, &align, &bitpos);
> + if (!vr.singleton_p ())
> + get_pointer_alignment_1 (arg, &align, &bitpos);
>
> if (align > BITS_PER_UNIT
> && opt_for_fn (cs->caller->decl, flag_ipa_bit_cp))
> {
> + unsigned prec = TYPE_PRECISION (TREE_TYPE (arg));
> wide_int mask
> = wi::bit_and_not (wi::mask (prec, false, prec),
> wide_int::from (align / BITS_PER_UNIT - 1,
> @@ -2411,12 +2410,10 @@ ipa_compute_jump_functions_for_edge (struct
> ipa_func_body_info *fbi,
> wide_int value = wide_int::from (bitpos / BITS_PER_UNIT, prec,
> UNSIGNED);
> irange_bitmask bm (value, mask);
> - if (!addr_nonzero)
> - vr.set_varying (TREE_TYPE (arg));
> vr.update_bitmask (bm);
> ipa_set_jfunc_vr (jfunc, vr);
> }
> - else if (addr_nonzero)
> + else if (!vr.varying_p ())
> ipa_set_jfunc_vr (jfunc, vr);
> else
> gcc_assert (!jfunc->m_vr);
> --
> 2.47.0
