| Message ID | ri6ilwfggp3.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | ipa: Careful processing ANCESTOR jump functions and NULL pointers (PR 103083) | expand |
> Hi, > > 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. > > (*) 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. I guess so. Thinking about it bit more after our discussion yesterday I think it really matters where the ADDR_EXPR was created (since in funciton with -fno-delete-null-pointer-checks it might be equal to 0) and we need to somehow keep track of it. One observation is that it is unsafe to constant propagate ADDR_EXPR with -fno-delete-null-pointer-checks to function with -fdelete-null-pointer-checks, so we may just simply stop propagating known ADDR_EXPRs on when caller is -fno... and call is -f.... > +/* 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::eq_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0); There is also ne_p > @@ -2374,6 +2384,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, > tree operand = NULL_TREE; > enum tree_code code; > unsigned src_idx; > + bool only_for_nonzero = false; > > if (jfunc->type == IPA_JF_PASS_THROUGH) > { > @@ -2386,7 +2397,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; I still think the offset should be in bytes (and probably poly_int64). There is get_addr_base_and_unit_offset > + only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset); > operand = build_int_cstu (size_type_node, offset); > } > > @@ -2404,16 +2417,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, > 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); > + || (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. > @@ -3327,7 +3332,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; Somewhere here you need to consider the case where you have two accessor jump functions to combine together and merge the keep_null flags... Also with the flags, I think you want to modify ipa-cp so NULL pointers gets propagated acroess the ancestor jump functions. Moreover ipa-modref is using ancestor jf to update its summary. Here I think with -fno-delete-null-pointer-checks it is safe to pdate also with the flag set, since we know the memory access is invalid otherwise. Honza > default: > @@ -4758,6 +4764,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 +4890,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 42842d9466a..8f0039d1ef8 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.c b/gcc/testsuite/gcc.dg/ipa/pr103083.c > new file mode 100644 > index 00000000000..e2fbb45d3cc > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr103083.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; > +} > -- > 2.33.1 >
Hi, On Sat, Nov 27 2021, Jan Hubicka wrote: >> Hi, >> >> 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. >> >> (*) 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. > > I guess so. Thinking about it bit more after our discussion yesterday > I think it really matters where the ADDR_EXPR was created (since in > funciton with -fno-delete-null-pointer-checks it might be equal to 0) > and we need to somehow keep track of it. > > One observation is that it is unsafe to constant propagate > ADDR_EXPR with -fno-delete-null-pointer-checks to function with > -fdelete-null-pointer-checks, so we may just simply stop propagating > known ADDR_EXPRs on when caller is -fno... and call is -f.... Makes sense. >> +/* 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::eq_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0); > There is also ne_p Changed. >> @@ -2374,6 +2384,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, >> tree operand = NULL_TREE; >> enum tree_code code; >> unsigned src_idx; >> + bool only_for_nonzero = false; >> >> if (jfunc->type == IPA_JF_PASS_THROUGH) >> { >> @@ -2386,7 +2397,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; > I still think the offset should be in bytes (and probably poly_int64). > There is get_addr_base_and_unit_offset I agree about bytes, not sure about poly_ints, but could be persuaded. But probably not as a part of this patch? > >> + only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset); >> operand = build_int_cstu (size_type_node, offset); >> } >> >> @@ -2404,16 +2417,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, >> 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); >> + || (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. >> @@ -3327,7 +3332,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; > Somewhere here you need to consider the case where you have two accessor > jump functions to combine together and merge the keep_null flags... Oops, I missed that, fixed. > > Also with the flags, I think you want to modify ipa-cp so NULL pointers > gets propagated acroess the ancestor jump functions. OK, added, along with a new testcase. > Moreover ipa-modref is using ancestor jf to update its summary. Here I > think with -fno-delete-null-pointer-checks it is safe to pdate also with > the flag set, since we know the memory access is invalid otherwise. I think it currently is, but only because for: int array_at_addr_zero[2]; void __attribute__((noinline)) bar(int *p) { clobber(p ? &p[1] : p); } /* ... */ bar(array_at_addr_zero); we do not produce an ancestor jump function (or any jump function whatsoever). In the IL it is not represented as an ADDR_EXPR of a MEM_REF but as a POINTER_PLUS. Anyway, I guess I'll need to amend it to avoid the set_to_bottom you claim is not necessary but currently I have the following. Thanks, Martin gcc/ChangeLog: 2021-11-29 Martin Jambor <mjambor@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. (propagate_bits_across_jump_function): Only process ancestor functions when safe. Remove extraneous condition handling ancestor jump functions. (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 <mjambor@suse.cz> * gcc.dg/ipa/pr103083-1.c: New test. * gcc.dg/ipa/pr103083-2.c: Likewise. --- gcc/ipa-cp.c | 42 +++++++++++++++++++-------- gcc/ipa-prop.c | 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, 116 insertions(+), 17 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.c b/gcc/ipa-cp.c index 5d9bb974d6b..c19fe9cf2ea 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -306,14 +306,15 @@ 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); @@ -1081,6 +1082,15 @@ 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 @@ -1477,6 +1487,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 +2386,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, tree operand = NULL_TREE; enum tree_code code; unsigned src_idx; + bool only_for_nonzero = false; if (jfunc->type == IPA_JF_PASS_THROUGH) { @@ -2385,7 +2399,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; + only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset); operand = build_int_cstu (size_type_node, offset); } @@ -2403,16 +2419,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, 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); + || (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 (); + } else return dest_lattice->meet_with (src_lats->bits_lattice, precision, sgn, code, operand); } - - else if (jfunc->type == IPA_JF_ANCESTOR) - return dest_lattice->set_to_bottom (); else if (jfunc->bits) return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask, precision); diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index bc5643206b9..ef9e353764c 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -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 ba49843a510..4e243bcfe19 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" } } */
Hello, I would like to ping the patch below. Martin On Mon, Nov 29 2021, Martin Jambor wrote: > Hi, > > On Sat, Nov 27 2021, Jan Hubicka wrote: >>> Hi, >>> >>> 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. >>> >>> (*) 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. >> >> I guess so. Thinking about it bit more after our discussion yesterday >> I think it really matters where the ADDR_EXPR was created (since in >> funciton with -fno-delete-null-pointer-checks it might be equal to 0) >> and we need to somehow keep track of it. >> >> One observation is that it is unsafe to constant propagate >> ADDR_EXPR with -fno-delete-null-pointer-checks to function with >> -fdelete-null-pointer-checks, so we may just simply stop propagating >> known ADDR_EXPRs on when caller is -fno... and call is -f.... > > Makes sense. > >>> +/* 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::eq_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0); >> There is also ne_p > > Changed. > >>> @@ -2374,6 +2384,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, >>> tree operand = NULL_TREE; >>> enum tree_code code; >>> unsigned src_idx; >>> + bool only_for_nonzero = false; >>> >>> if (jfunc->type == IPA_JF_PASS_THROUGH) >>> { >>> @@ -2386,7 +2397,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; >> I still think the offset should be in bytes (and probably poly_int64). >> There is get_addr_base_and_unit_offset > > I agree about bytes, not sure about poly_ints, but could be persuaded. > But probably not as a part of this patch? > >> >>> + only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset); >>> operand = build_int_cstu (size_type_node, offset); >>> } >>> >>> @@ -2404,16 +2417,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, >>> 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); >>> + || (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. > >>> @@ -3327,7 +3332,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; >> Somewhere here you need to consider the case where you have two accessor >> jump functions to combine together and merge the keep_null flags... > > Oops, I missed that, fixed. > >> >> Also with the flags, I think you want to modify ipa-cp so NULL pointers >> gets propagated acroess the ancestor jump functions. > > OK, added, along with a new testcase. > >> Moreover ipa-modref is using ancestor jf to update its summary. Here I >> think with -fno-delete-null-pointer-checks it is safe to pdate also with >> the flag set, since we know the memory access is invalid otherwise. > > I think it currently is, but only because for: > > int array_at_addr_zero[2]; > > void __attribute__((noinline)) > bar(int *p) > { > clobber(p ? &p[1] : p); > } > /* ... */ > bar(array_at_addr_zero); > > we do not produce an ancestor jump function (or any jump function > whatsoever). In the IL it is not represented as an ADDR_EXPR of a > MEM_REF but as a POINTER_PLUS. > > Anyway, I guess I'll need to amend it to avoid the set_to_bottom you > claim is not necessary but currently I have the following. > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2021-11-29 Martin Jambor <mjambor@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. > (propagate_bits_across_jump_function): Only process ancestor functions > when safe. Remove extraneous condition handling ancestor jump > functions. > (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 <mjambor@suse.cz> > > * gcc.dg/ipa/pr103083-1.c: New test. > * gcc.dg/ipa/pr103083-2.c: Likewise. > --- > gcc/ipa-cp.c | 42 +++++++++++++++++++-------- > gcc/ipa-prop.c | 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, 116 insertions(+), 17 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.c b/gcc/ipa-cp.c > index 5d9bb974d6b..c19fe9cf2ea 100644 > --- a/gcc/ipa-cp.c > +++ b/gcc/ipa-cp.c > @@ -306,14 +306,15 @@ 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); > @@ -1081,6 +1082,15 @@ 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 > @@ -1477,6 +1487,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 +2386,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, > tree operand = NULL_TREE; > enum tree_code code; > unsigned src_idx; > + bool only_for_nonzero = false; > > if (jfunc->type == IPA_JF_PASS_THROUGH) > { > @@ -2385,7 +2399,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; > + only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset); > operand = build_int_cstu (size_type_node, offset); > } > > @@ -2403,16 +2419,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, > 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); > + || (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 (); > + } > else > return dest_lattice->meet_with (src_lats->bits_lattice, precision, sgn, > code, operand); > } > - > - else if (jfunc->type == IPA_JF_ANCESTOR) > - return dest_lattice->set_to_bottom (); > else if (jfunc->bits) > return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask, > precision); > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c > index bc5643206b9..ef9e353764c 100644 > --- a/gcc/ipa-prop.c > +++ b/gcc/ipa-prop.c > @@ -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 ba49843a510..4e243bcfe19 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.33.1
> >>> + || (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? 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 > or all known bits of src are zero but then there are unknown masked-out bits with unknown value and in that case we just don't know. The patch does src+offset when there is a known non-zero bit but we can only set dest to zero if all bits are known. But as you pointed out, the constant part of IPA-CP should catch this case, and now does, so I decided not to handle it here. If you prefer, I can add a case if both m_value and m_mask or zero (well, could the latter in theory only need "precision" zeros)? and if so, simply meet the destination lattice with the source one. Is that what you are asking for? Thanks, Martin
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 <mjambor@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 <mjambor@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" } } */
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 <mjambor@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 <mjambor@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
> 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 <mjambor@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 <mjambor@suse.cz> > > * gcc.dg/ipa/pr103083-1.c: New test. > * gcc.dg/ipa/pr103083-2.c: Likewise. OK, thanks! Honza
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index 703541d15cc..7e94bd78b98 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -306,14 +306,15 @@ 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); @@ -1081,6 +1082,15 @@ 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::eq_p (wi::bit_and (wi::bit_not (m_mask), m_value), 0); +} + /* Convert operand to value, mask form. */ void @@ -2374,6 +2384,7 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, tree operand = NULL_TREE; enum tree_code code; unsigned src_idx; + bool only_for_nonzero = false; if (jfunc->type == IPA_JF_PASS_THROUGH) { @@ -2386,7 +2397,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; + only_for_nonzero = (ipa_get_jf_ancestor_keep_null (jfunc) || !offset); operand = build_int_cstu (size_type_node, offset); } @@ -2404,16 +2417,18 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx, 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); + || (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 (); + } else return dest_lattice->meet_with (src_lats->bits_lattice, precision, sgn, code, operand); } - - else if (jfunc->type == IPA_JF_ANCESTOR) - return dest_lattice->set_to_bottom (); else if (jfunc->bits) return dest_lattice->meet_with (jfunc->bits->value, jfunc->bits->mask, precision); diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index e85df0971fc..7d19e30e3f5 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -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 @@ -3327,7 +3332,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 +4764,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 +4890,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 42842d9466a..8f0039d1ef8 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.c b/gcc/testsuite/gcc.dg/ipa/pr103083.c new file mode 100644 index 00000000000..e2fbb45d3cc --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr103083.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; +}