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 <mjam...@suse.cz> > > 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 <mjam...@suse.cz> > > * 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