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.
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
