Hello,
I would like to ping this patch, please.
Thanks,
Martin
On Mon, Feb 14 2022, Martin Jambor wrote:
> Hello Honza,
>
> On Mon, Dec 13 2021, Jan Hubicka wrote:
>>> >>> + || (only_for_nonzero &&
>>> >>> !src_lats->bits_lattice.known_nonzero_p ()))
>>> >>> + {
>>> >>> + if (jfunc->bits)
>>> >>> + return dest_lattice->meet_with (jfunc->bits->value,
>>> >>> + jfunc->bits->mask,
>>> >>> precision);
>>> >>> + else
>>> >>> + return dest_lattice->set_to_bottom ();
>>> >>> + }
>>> >> I do not think you need to set to bottom here. For pointers, we
>>> >> primarily track alignment and NULL is aligned - all you need to do is to
>>> >> make sure that we do not believe that some bits are 1.
>>> >
>>> > I am not sure I understand, the testcase you wrote has all bits as zeros
>>> > and still miscompiles? It is primarily used for alignment but not only
>>> > for that.
>>
>> Maybe I misunderstand the code. But if you have only_for_nonzero and
>> you do not know htat src lattice is non-zero you will get
>> - if src is 0, then dest is 0
>> - if src is non-zero then dest is src+offset
>>
>> So all trialing bits that are known to be 0 in src and offset are known
>> to be 0 in the result. But I do not see where th eoffset is mixed in?
>>
>
> I went back and tried to figure out again what you meant and I hope it
> was the following patch, which does not drop the lattice to bottom for
> ANCESTORs but instead masks any bits that would be considered known
> ones. It is indeed clever and I did not see the possibility when
> writing the patch.
>
> The following has passed bootstrap, LTO bootstrap and testing on
> x86-64. OK for trunk (and then to be backported to 11 and 10)?
>
> Thanks,
>
> Martin
>
>
> IPA_JF_ANCESTOR jump functions are constructed also when the formal
> parameter of the caller is first checked whether it is NULL and left
> as it is if it is NULL, to accommodate C++ casts to an ancestor class.
>
> The jump function type was invented for devirtualization and IPA-CP
> propagation of tree constants is also careful to apply it only to
> existing DECLs(*) but as PR 103083 shows, the part propagating "known
> bits" was not careful about this, which can lead to miscompilations.
>
> This patch introduces a flag to the ancestor jump functions which
> tells whether a NULL-check was elided when creating it and makes the
> bits propagation behave accordingly, masking any bits otherwise would
> be known to be one. This should safely preserve alignment info, which
> is the primary ifnormation that we keep in bits for pointers.
>
> (*) There still may remain problems when a DECL resides on address
> zero (with -fno-delete-null-pointer-checks ...I hope it cannot happen
> otherwise). I am looking into that now but I think it will be easier
> for everyone if I do so in a follow-up patch.
>
> gcc/ChangeLog:
>
> 2022-02-11 Martin Jambor <[email protected]>
>
> PR ipa/103083
> * ipa-prop.h (ipa_ancestor_jf_data): New flag keep_null;
> (ipa_get_jf_ancestor_keep_null): New function.
> * ipa-prop.c (ipa_set_ancestor_jf): Initialize keep_null field of the
> ancestor function.
> (compute_complex_assign_jump_func): Pass false to keep_null
> parameter of ipa_set_ancestor_jf.
> (compute_complex_ancestor_jump_func): Pass true to keep_null
> parameter of ipa_set_ancestor_jf.
> (update_jump_functions_after_inlining): Carry over keep_null from the
> original ancestor jump-function or merge them.
> (ipa_write_jump_function): Stream keep_null flag.
> (ipa_read_jump_function): Likewise.
> (ipa_print_node_jump_functions_for_edge): Print the new flag.
> * ipa-cp.c (class ipcp_bits_lattice): Make various getters const. New
> member function known_nonzero_p.
> (ipcp_bits_lattice::known_nonzero_p): New.
> (ipcp_bits_lattice::meet_with_1): New parameter drop_all_ones,
> observe it.
> (ipcp_bits_lattice::meet_with): Likewise.
> (propagate_bits_across_jump_function): Simplify. Pass true in
> drop_all_ones when it is necessary.
> (propagate_aggs_across_jump_function): Take care of keep_null
> flag.
> (ipa_get_jf_ancestor_result): Propagate NULL accross keep_null
> jump functions.
>
> gcc/testsuite/ChangeLog:
>
> 2021-11-25 Martin Jambor <[email protected]>
>
> * gcc.dg/ipa/pr103083-1.c: New test.
> * gcc.dg/ipa/pr103083-2.c: Likewise.
> ---
> gcc/ipa-cp.cc | 75 ++++++++++++++++++---------
> gcc/ipa-prop.cc | 20 +++++--
> gcc/ipa-prop.h | 13 +++++
> gcc/testsuite/gcc.dg/ipa/pr103083-1.c | 28 ++++++++++
> gcc/testsuite/gcc.dg/ipa/pr103083-2.c | 30 +++++++++++
> 5 files changed, 137 insertions(+), 29 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083-1.c
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083-2.c
>
> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
> index 453e9c93cc3..93a54ae83fc 100644
> --- a/gcc/ipa-cp.cc
> +++ b/gcc/ipa-cp.cc
> @@ -306,17 +306,18 @@ public:
> class ipcp_bits_lattice
> {
> public:
> - bool bottom_p () { return m_lattice_val == IPA_BITS_VARYING; }
> - bool top_p () { return m_lattice_val == IPA_BITS_UNDEFINED; }
> - bool constant_p () { return m_lattice_val == IPA_BITS_CONSTANT; }
> + bool bottom_p () const { return m_lattice_val == IPA_BITS_VARYING; }
> + bool top_p () const { return m_lattice_val == IPA_BITS_UNDEFINED; }
> + bool constant_p () const { return m_lattice_val == IPA_BITS_CONSTANT; }
> bool set_to_bottom ();
> bool set_to_constant (widest_int, widest_int);
> + bool known_nonzero_p () const;
>
> - widest_int get_value () { return m_value; }
> - widest_int get_mask () { return m_mask; }
> + widest_int get_value () const { return m_value; }
> + widest_int get_mask () const { return m_mask; }
>
> bool meet_with (ipcp_bits_lattice& other, unsigned, signop,
> - enum tree_code, tree);
> + enum tree_code, tree, bool);
>
> bool meet_with (widest_int, widest_int, unsigned);
>
> @@ -330,7 +331,7 @@ private:
> value is known to be constant. */
> widest_int m_value, m_mask;
>
> - bool meet_with_1 (widest_int, widest_int, unsigned);
> + bool meet_with_1 (widest_int, widest_int, unsigned, bool);
> void get_value_and_mask (tree, widest_int *, widest_int *);
> };
>
> @@ -1081,6 +1082,16 @@ ipcp_bits_lattice::set_to_constant (widest_int value,
> widest_int mask)
> return true;
> }
>
> +/* Return true if any of the known bits are non-zero. */
> +
> +bool
> +ipcp_bits_lattice::known_nonzero_p () const
> +{
> + if (!constant_p ())
> + return false;
> + return wi::ne_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0);
> +}
> +
> /* Convert operand to value, mask form. */
>
> void
> @@ -1103,16 +1114,19 @@ ipcp_bits_lattice::get_value_and_mask (tree operand,
> widest_int *valuep, widest_
> /* Meet operation, similar to ccp_lattice_meet, we xor values
> if this->value, value have different values at same bit positions, we want
> to drop that bit to varying. Return true if mask is changed.
> - This function assumes that the lattice value is in CONSTANT state */
> + This function assumes that the lattice value is in CONSTANT state. If
> + DROP_ALL_ONES, mask out any known bits with value one afterwards. */
>
> bool
> ipcp_bits_lattice::meet_with_1 (widest_int value, widest_int mask,
> - unsigned precision)
> + unsigned precision, bool drop_all_ones)
> {
> gcc_assert (constant_p ());
>
> widest_int old_mask = m_mask;
> m_mask = (m_mask | mask) | (m_value ^ value);
> + if (drop_all_ones)
> + m_mask |= m_value;
> m_value &= ~m_mask;
>
> if (wi::sext (m_mask, precision) == -1)
> @@ -1138,16 +1152,18 @@ ipcp_bits_lattice::meet_with (widest_int value,
> widest_int mask,
> return set_to_constant (value, mask);
> }
>
> - return meet_with_1 (value, mask, precision);
> + return meet_with_1 (value, mask, precision, false);
> }
>
> /* Meet bits lattice with the result of bit_value_binop (other, operand)
> if code is binary operation or bit_value_unop (other) if code is unary op.
> - In the case when code is nop_expr, no adjustment is required. */
> + In the case when code is nop_expr, no adjustment is required. If
> + DROP_ALL_ONES, mask out any known bits with value one afterwards. */
>
> bool
> ipcp_bits_lattice::meet_with (ipcp_bits_lattice& other, unsigned precision,
> - signop sgn, enum tree_code code, tree operand)
> + signop sgn, enum tree_code code, tree operand,
> + bool drop_all_ones)
> {
> if (other.bottom_p ())
> return set_to_bottom ();
> @@ -1186,12 +1202,18 @@ ipcp_bits_lattice::meet_with (ipcp_bits_lattice&
> other, unsigned precision,
>
> if (top_p ())
> {
> + if (drop_all_ones)
> + {
> + adjusted_mask |= adjusted_value;
> + adjusted_value &= ~adjusted_mask;
> + }
> if (wi::sext (adjusted_mask, precision) == -1)
> return set_to_bottom ();
> return set_to_constant (adjusted_value, adjusted_mask);
> }
> else
> - return meet_with_1 (adjusted_value, adjusted_mask, precision);
> + return meet_with_1 (adjusted_value, adjusted_mask, precision,
> + drop_all_ones);
> }
>
> /* Mark bot aggregate and scalar lattices as containing an unknown variable,
> @@ -1477,6 +1499,9 @@ ipa_get_jf_ancestor_result (struct ipa_jump_func
> *jfunc, tree input)
> fold_build2 (MEM_REF, TREE_TYPE (TREE_TYPE (input)), input,
> build_int_cst (ptr_type_node, byte_offset)));
> }
> + else if (ipa_get_jf_ancestor_keep_null (jfunc)
> + && zerop (input))
> + return input;
> else
> return NULL_TREE;
> }
> @@ -2373,6 +2398,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs,
> int idx,
> tree operand = NULL_TREE;
> enum tree_code code;
> unsigned src_idx;
> + bool keep_null = false;
>
> if (jfunc->type == IPA_JF_PASS_THROUGH)
> {
> @@ -2385,7 +2411,9 @@ propagate_bits_across_jump_function (cgraph_edge *cs,
> int idx,
> {
> code = POINTER_PLUS_EXPR;
> src_idx = ipa_get_jf_ancestor_formal_id (jfunc);
> - unsigned HOST_WIDE_INT offset = ipa_get_jf_ancestor_offset (jfunc) /
> BITS_PER_UNIT;
> + unsigned HOST_WIDE_INT offset
> + = ipa_get_jf_ancestor_offset (jfunc) / BITS_PER_UNIT;
> + keep_null = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset);
> operand = build_int_cstu (size_type_node, offset);
> }
>
> @@ -2402,18 +2430,17 @@ propagate_bits_across_jump_function (cgraph_edge *cs,
> int idx,
> result of x & 0xff == 0xff, which gets computed during ccp1 pass
> and we store it in jump function during analysis stage. */
>
> - if (src_lats->bits_lattice.bottom_p ()
> - && jfunc->bits)
> - return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask,
> - precision);
> - else
> - return dest_lattice->meet_with (src_lats->bits_lattice, precision, sgn,
> - code, operand);
> + if (!src_lats->bits_lattice.bottom_p ())
> + {
> + bool drop_all_ones
> + = keep_null && !src_lats->bits_lattice.known_nonzero_p ();
> +
> + return dest_lattice->meet_with (src_lats->bits_lattice, precision,
> + sgn, code, operand, drop_all_ones);
> + }
> }
>
> - else if (jfunc->type == IPA_JF_ANCESTOR)
> - return dest_lattice->set_to_bottom ();
> - else if (jfunc->bits)
> + if (jfunc->bits)
> return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask,
> precision);
> else
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index e55fe2776f2..70c073b42a6 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -357,6 +357,8 @@ ipa_print_node_jump_functions_for_edge (FILE *f, struct
> cgraph_edge *cs)
> jump_func->value.ancestor.offset);
> if (jump_func->value.ancestor.agg_preserved)
> fprintf (f, ", agg_preserved");
> + if (jump_func->value.ancestor.keep_null)
> + fprintf (f, ", keep_null");
> fprintf (f, "\n");
> }
>
> @@ -601,12 +603,13 @@ ipa_set_jf_arith_pass_through (struct ipa_jump_func
> *jfunc, int formal_id,
>
> static void
> ipa_set_ancestor_jf (struct ipa_jump_func *jfunc, HOST_WIDE_INT offset,
> - int formal_id, bool agg_preserved)
> + int formal_id, bool agg_preserved, bool keep_null)
> {
> jfunc->type = IPA_JF_ANCESTOR;
> jfunc->value.ancestor.formal_id = formal_id;
> jfunc->value.ancestor.offset = offset;
> jfunc->value.ancestor.agg_preserved = agg_preserved;
> + jfunc->value.ancestor.keep_null = keep_null;
> }
>
> /* Get IPA BB information about the given BB. FBI is the context of analyzis
> @@ -1438,7 +1441,8 @@ compute_complex_assign_jump_func (struct
> ipa_func_body_info *fbi,
> index = ipa_get_param_decl_index (info, SSA_NAME_VAR (ssa));
> if (index >= 0 && param_type && POINTER_TYPE_P (param_type))
> ipa_set_ancestor_jf (jfunc, offset, index,
> - parm_ref_data_pass_through_p (fbi, index, call, ssa));
> + parm_ref_data_pass_through_p (fbi, index, call, ssa),
> + false);
> }
>
> /* Extract the base, offset and MEM_REF expression from a statement ASSIGN if
> @@ -1564,7 +1568,8 @@ compute_complex_ancestor_jump_func (struct
> ipa_func_body_info *fbi,
> }
>
> ipa_set_ancestor_jf (jfunc, offset, index,
> - parm_ref_data_pass_through_p (fbi, index, call, parm));
> + parm_ref_data_pass_through_p (fbi, index, call, parm),
> + true);
> }
>
> /* Inspect the given TYPE and return true iff it has the same structure (the
> @@ -3250,6 +3255,7 @@ update_jump_functions_after_inlining (struct
> cgraph_edge *cs,
> dst->value.ancestor.offset += src->value.ancestor.offset;
> dst->value.ancestor.agg_preserved &=
> src->value.ancestor.agg_preserved;
> + dst->value.ancestor.keep_null |= src->value.ancestor.keep_null;
> }
> else
> ipa_set_jf_unknown (dst);
> @@ -3327,7 +3333,8 @@ update_jump_functions_after_inlining (struct
> cgraph_edge *cs,
> ipa_set_ancestor_jf (dst,
> ipa_get_jf_ancestor_offset (src),
> ipa_get_jf_ancestor_formal_id (src),
> - agg_p);
> + agg_p,
> + ipa_get_jf_ancestor_keep_null (src));
> break;
> }
> default:
> @@ -4758,6 +4765,7 @@ ipa_write_jump_function (struct output_block *ob,
> streamer_write_uhwi (ob, jump_func->value.ancestor.formal_id);
> bp = bitpack_create (ob->main_stream);
> bp_pack_value (&bp, jump_func->value.ancestor.agg_preserved, 1);
> + bp_pack_value (&bp, jump_func->value.ancestor.keep_null, 1);
> streamer_write_bitpack (&bp);
> break;
> default:
> @@ -4883,7 +4891,9 @@ ipa_read_jump_function (class lto_input_block *ib,
> int formal_id = streamer_read_uhwi (ib);
> struct bitpack_d bp = streamer_read_bitpack (ib);
> bool agg_preserved = bp_unpack_value (&bp, 1);
> - ipa_set_ancestor_jf (jump_func, offset, formal_id, agg_preserved);
> + bool keep_null = bp_unpack_value (&bp, 1);
> + ipa_set_ancestor_jf (jump_func, offset, formal_id, agg_preserved,
> + keep_null);
> break;
> }
> default:
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index 553adfc9f35..b22dfb5315c 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -143,6 +143,8 @@ struct GTY(()) ipa_ancestor_jf_data
> int formal_id;
> /* Flag with the same meaning like agg_preserve in ipa_pass_through_data.
> */
> unsigned agg_preserved : 1;
> + /* When set, the operation should not have any effect on NULL pointers. */
> + unsigned keep_null : 1;
> };
>
> /* A jump function for an aggregate part at a given offset, which describes
> how
> @@ -438,6 +440,17 @@ ipa_get_jf_ancestor_type_preserved (struct ipa_jump_func
> *jfunc)
> return jfunc->value.ancestor.agg_preserved;
> }
>
> +/* Return if jfunc represents an operation whether we first check the formal
> + parameter for non-NULLness unless it does not matter because the offset is
> + zero anyway. */
> +
> +static inline bool
> +ipa_get_jf_ancestor_keep_null (struct ipa_jump_func *jfunc)
> +{
> + gcc_checking_assert (jfunc->type == IPA_JF_ANCESTOR);
> + return jfunc->value.ancestor.keep_null;
> +}
> +
> /* Class for allocating a bundle of various potentially known properties
> about
> actual arguments of a particular call on stack for the usual case and on
> heap only if there are unusually many arguments. The data is deallocated
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr103083-1.c
> b/gcc/testsuite/gcc.dg/ipa/pr103083-1.c
> new file mode 100644
> index 00000000000..e2fbb45d3cc
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr103083-1.c
> @@ -0,0 +1,28 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -Wno-pointer-to-int-cast" } */
> +
> +struct b {int b;};
> +struct a {int a; struct b b;};
> +
> +long i;
> +
> +__attribute__ ((noinline))
> +static void test2 (struct b *b)
> +{
> + if (((int)b)&4)
> + __builtin_abort ();
> +}
> +
> +__attribute__ ((noinline))
> +static void
> +test (struct a *a)
> +{
> + test2(a? &a->b : 0);
> +}
> +
> +int
> +main()
> +{
> + test(0);
> + return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr103083-2.c
> b/gcc/testsuite/gcc.dg/ipa/pr103083-2.c
> new file mode 100644
> index 00000000000..ae1b905af81
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr103083-2.c
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fno-ipa-bit-cp -fdump-tree-optimized" } */
> +
> +struct b {int b;};
> +struct a {int a; struct b b;};
> +
> +void remove_any_mention (void);
> +
> +__attribute__ ((noinline))
> +static void test2 (struct b *b)
> +{
> + if (b)
> + remove_any_mention ();
> +}
> +
> +__attribute__ ((noinline))
> +static void
> +test (struct a *a)
> +{
> + test2(a? &a->b : 0);
> +}
> +
> +int
> +foo()
> +{
> + test(0);
> + return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "remove_any_mention" "optimized" } } */
> --
> 2.34.1
