| Message ID | CAAgBjMkR_Am6Zn26zx+Ah7AHZjhKe8J+156VWxZG74C+QZFADw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 21, 2016 at 6:44 PM, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: > Hi, > The attached patch tries to extend ipa bits propagation to handle > pointer alignment propagation. > The patch just disables ipa-cp-alignment pass, I suppose we want to > eventually remove it ? > > Bootstrap+tested on x86_64-unknown-linux-gnu. > Cross-tested on arm*-*-*, aarch64*-*-*. > Does the patch look OK ? Just looking at the alignment extraction: + else if (POINTER_TYPE_P (...)) + { + unsigned tem = bits[i].mask.to_uhwi (); + unsigned HOST_WIDE_INT bitpos = bits[i].value.to_uhwi (); + unsigned align = tem & -tem; + unsigned misalign = bitpos & (align - 1); ... + if (old_known + && old_align > align) + { + if (dump_file) + fprintf (dump_file, "But alignment was already %u.\n", old_align); + continue; + } it would be nice to sanity check old misalign against misalign. Basically gcc_assert (misalign & (old_align - 1) == old_misalign) here (and in the old_align > align case the reverse). + set_ptr_info_alignment (pi, align, misalign); - ret |= propagate_alignment_accross_jump_function (cs, jump_func, - &dest_plats->alignment); +// ret |= propagate_alignment_accross_jump_function (cs, jump_func, +// this should of course be removed rather than commented. Leaving the IPA parts to somebody else. Richard. > Thanks, > Prathamesh
On Thu, Sep 22, 2016 at 11:21 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Wed, Sep 21, 2016 at 6:44 PM, Prathamesh Kulkarni > <prathamesh.kulkarni@linaro.org> wrote: >> Hi, >> The attached patch tries to extend ipa bits propagation to handle >> pointer alignment propagation. >> The patch just disables ipa-cp-alignment pass, I suppose we want to >> eventually remove it ? >> >> Bootstrap+tested on x86_64-unknown-linux-gnu. >> Cross-tested on arm*-*-*, aarch64*-*-*. >> Does the patch look OK ? > > Just looking at the alignment extraction: > > + else > > if (POINTER_TYPE_P (...)) > > + { > + unsigned tem = bits[i].mask.to_uhwi (); > + unsigned HOST_WIDE_INT bitpos = bits[i].value.to_uhwi (); > + unsigned align = tem & -tem; > + unsigned misalign = bitpos & (align - 1); > ... > + if (old_known > + && old_align > align) > + { > + if (dump_file) > + fprintf (dump_file, "But alignment was already %u.\n", > old_align); > + continue; > + } > > it would be nice to sanity check old misalign against misalign. > Basically > > gcc_assert (misalign & (old_align - 1) == old_misalign) > > here (and in the old_align > align case the reverse). Just occured to me that this might trigger on invalid user code where correct misalignment gets propagated in IPA but local bogus alignment doesn't cause an issue because we're on a ! strict-align target. So maybe instead of asserting output a warning here ... or at least put sth in the dumpfile. > + set_ptr_info_alignment (pi, align, misalign); > > > - ret |= propagate_alignment_accross_jump_function (cs, jump_func, > - > &dest_plats->alignment); > +// ret |= propagate_alignment_accross_jump_function (cs, jump_func, > +// > > this should of course be removed rather than commented. > > Leaving the IPA parts to somebody else. > > Richard. > > >> Thanks, >> Prathamesh
> Hi, > The attached patch tries to extend ipa bits propagation to handle > pointer alignment propagation. > The patch just disables ipa-cp-alignment pass, I suppose we want to > eventually remove it ? Yes, can you please verify that alignments it computes are monotonously worse than those your new code computes and include the removal in the next iteration of the patch? > > Bootstrap+tested on x86_64-unknown-linux-gnu. > Cross-tested on arm*-*-*, aarch64*-*-*. > Does the patch look OK ? > > Thanks, > Prathamesh > @@ -2258,8 +2271,8 @@ propagate_constants_accross_call (struct cgraph_edge *cs) > &dest_plats->itself); > ret |= propagate_context_accross_jump_function (cs, jump_func, i, > &dest_plats->ctxlat); > - ret |= propagate_alignment_accross_jump_function (cs, jump_func, > - &dest_plats->alignment); > +// ret |= propagate_alignment_accross_jump_function (cs, jump_func, > +// &dest_plats->alignment); obviously we do not want commented out ocde.. > ret |= propagate_bits_accross_jump_function (cs, i, jump_func, > &dest_plats->bits_lattice); > ret |= propagate_aggs_accross_jump_function (cs, jump_func, > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c > index 1629781..5cee27b 100644 > --- a/gcc/ipa-prop.c > +++ b/gcc/ipa-prop.c > @@ -1701,6 +1701,16 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi, > jfunc->bits.mask = 0; > } > } > + else if (POINTER_TYPE_P (TREE_TYPE (arg))) > + { > + unsigned HOST_WIDE_INT bitpos; > + unsigned align; > + > + jfunc->bits.known = true; > + get_pointer_alignment_1 (arg, &align, &bitpos); > + jfunc->bits.mask = wi::mask<widest_int>(TYPE_PRECISION (TREE_TYPE (arg)), false).and_not (align / BITS_PER_UNIT - 1); ... and long lines :) > + jfunc->bits.value = bitpos / BITS_PER_UNIT; > + } > else > gcc_assert (!jfunc->bits.known); > > @@ -5534,7 +5544,7 @@ ipcp_update_bits (struct cgraph_node *node) > next_parm = DECL_CHAIN (parm); > > if (!bits[i].known > - || !INTEGRAL_TYPE_P (TREE_TYPE (parm)) > + || !(INTEGRAL_TYPE_P (TREE_TYPE (parm)) || POINTER_TYPE_P (TREE_TYPE (parm))) I suppose eventually we may want to enable other types, too. It does even make sense to propagate this on aggregates, but definitly on vectors and complex numbers. Otherwise the patch seems fine to me (modulo Richard's comments) Honza
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index 5ff7bed..d251223 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -1871,16 +1871,29 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j unsigned precision = TYPE_PRECISION (parm_type); signop sgn = TYPE_SIGN (parm_type); - if (jfunc->type == IPA_JF_PASS_THROUGH) + if (jfunc->type == IPA_JF_PASS_THROUGH + || jfunc->type == IPA_JF_ANCESTOR) { struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller); - enum tree_code code = ipa_get_jf_pass_through_operation (jfunc); tree operand = NULL_TREE; + enum tree_code code; + unsigned src_idx; - if (code != NOP_EXPR) - operand = ipa_get_jf_pass_through_operand (jfunc); + if (jfunc->type == IPA_JF_PASS_THROUGH) + { + code = ipa_get_jf_pass_through_operation (jfunc); + src_idx = ipa_get_jf_pass_through_formal_id (jfunc); + if (code != NOP_EXPR) + operand = ipa_get_jf_pass_through_operand (jfunc); + } + else + { + 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; + operand = build_int_cstu (size_type_node, offset); + } - int src_idx = ipa_get_jf_pass_through_formal_id (jfunc); struct ipcp_param_lattices *src_lats = ipa_get_parm_lattices (caller_info, src_idx); @@ -2258,8 +2271,8 @@ propagate_constants_accross_call (struct cgraph_edge *cs) &dest_plats->itself); ret |= propagate_context_accross_jump_function (cs, jump_func, i, &dest_plats->ctxlat); - ret |= propagate_alignment_accross_jump_function (cs, jump_func, - &dest_plats->alignment); +// ret |= propagate_alignment_accross_jump_function (cs, jump_func, +// &dest_plats->alignment); ret |= propagate_bits_accross_jump_function (cs, i, jump_func, &dest_plats->bits_lattice); ret |= propagate_aggs_accross_jump_function (cs, jump_func, diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 1629781..5cee27b 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -1701,6 +1701,16 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi, jfunc->bits.mask = 0; } } + else if (POINTER_TYPE_P (TREE_TYPE (arg))) + { + unsigned HOST_WIDE_INT bitpos; + unsigned align; + + jfunc->bits.known = true; + get_pointer_alignment_1 (arg, &align, &bitpos); + jfunc->bits.mask = wi::mask<widest_int>(TYPE_PRECISION (TREE_TYPE (arg)), false).and_not (align / BITS_PER_UNIT - 1); + jfunc->bits.value = bitpos / BITS_PER_UNIT; + } else gcc_assert (!jfunc->bits.known); @@ -5534,7 +5544,7 @@ ipcp_update_bits (struct cgraph_node *node) next_parm = DECL_CHAIN (parm); if (!bits[i].known - || !INTEGRAL_TYPE_P (TREE_TYPE (parm)) + || !(INTEGRAL_TYPE_P (TREE_TYPE (parm)) || POINTER_TYPE_P (TREE_TYPE (parm))) || !is_gimple_reg (parm)) continue; @@ -5549,12 +5559,41 @@ ipcp_update_bits (struct cgraph_node *node) fprintf (dump_file, "\n"); } - unsigned prec = TYPE_PRECISION (TREE_TYPE (ddef)); - signop sgn = TYPE_SIGN (TREE_TYPE (ddef)); + if (INTEGRAL_TYPE_P (TREE_TYPE (ddef))) + { + unsigned prec = TYPE_PRECISION (TREE_TYPE (ddef)); + signop sgn = TYPE_SIGN (TREE_TYPE (ddef)); + + wide_int nonzero_bits = wide_int::from (bits[i].mask, prec, UNSIGNED) + | wide_int::from (bits[i].value, prec, sgn); + set_nonzero_bits (ddef, nonzero_bits); + } + else + { + unsigned tem = bits[i].mask.to_uhwi (); + unsigned HOST_WIDE_INT bitpos = bits[i].value.to_uhwi (); + unsigned align = tem & -tem; + unsigned misalign = bitpos & (align - 1); - wide_int nonzero_bits = wide_int::from (bits[i].mask, prec, UNSIGNED) - | wide_int::from (bits[i].value, prec, sgn); - set_nonzero_bits (ddef, nonzero_bits); + if (align > 1) + { + if (dump_file) + fprintf (dump_file, "Adjusting align: %u, misalign: %u\n", align, misalign); + + unsigned old_align, old_misalign; + struct ptr_info_def *pi = get_ptr_info (ddef); + bool old_known = get_ptr_info_alignment (pi, &old_align, &old_misalign); + + if (old_known + && old_align > align) + { + if (dump_file) + fprintf (dump_file, "But alignment was already %u.\n", old_align); + continue; + } + set_ptr_info_alignment (pi, align, misalign); + } + } } } diff --git a/gcc/testsuite/gcc.dg/ipa/propalign-1.c b/gcc/testsuite/gcc.dg/ipa/propalign-1.c index f34552c..1491de8 100644 --- a/gcc/testsuite/gcc.dg/ipa/propalign-1.c +++ b/gcc/testsuite/gcc.dg/ipa/propalign-1.c @@ -27,5 +27,5 @@ bar (void) } -/* { dg-final { scan-ipa-dump "Adjusting alignment of param" "cp" } } */ +/* { dg-final { scan-ipa-dump "Adjusting align" "cp" } } */ /* { dg-final { scan-tree-dump-not "fail_the_test" "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/ipa/propalign-2.c b/gcc/testsuite/gcc.dg/ipa/propalign-2.c index 67b149a..51799c7 100644 --- a/gcc/testsuite/gcc.dg/ipa/propalign-2.c +++ b/gcc/testsuite/gcc.dg/ipa/propalign-2.c @@ -53,5 +53,5 @@ bar2 (void) through (c.buf); } -/* { dg-final { scan-ipa-dump "Adjusting alignment of param" "cp" } } */ +/* { dg-final { scan-ipa-dump "Adjusting align" "cp" } } */ /* { dg-final { scan-tree-dump-not "fail_the_test" "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/ipa/propalign-3.c b/gcc/testsuite/gcc.dg/ipa/propalign-3.c index d3bc2c4..4f5df4a 100644 --- a/gcc/testsuite/gcc.dg/ipa/propalign-3.c +++ b/gcc/testsuite/gcc.dg/ipa/propalign-3.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -fno-ipa-cp-alignment -fno-early-inlining -fdump-ipa-cp -fdump-tree-optimized" } */ +/* { dg-options "-O2 -fno-ipa-bit-cp -fno-early-inlining -fdump-ipa-cp -fdump-tree-optimized" } */ /* { dg-skip-if "No alignment restrictions" { { ! natural_alignment_32 } && { ! natural_alignment_64 } } } */ #include <stdint.h> @@ -53,5 +53,5 @@ bar2 (void) through (c.buf); } -/* { dg-final { scan-ipa-dump-not "Adjusting alignment of param" "cp" } } */ +/* { dg-final { scan-ipa-dump-not "align:" "cp" } } */ /* { dg-final { scan-tree-dump "fail_the_test" "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/ipa/propalign-4.c b/gcc/testsuite/gcc.dg/ipa/propalign-4.c index b680813..bd32bf0 100644 --- a/gcc/testsuite/gcc.dg/ipa/propalign-4.c +++ b/gcc/testsuite/gcc.dg/ipa/propalign-4.c @@ -20,4 +20,4 @@ main() test (&aa[3]); return 0; } -/* { dg-final { scan-ipa-dump "Alignment 8, misalignment 4" "cp" } } */ +/* { dg-final { scan-ipa-dump "align: 8, misalign: 4" "cp" } } */ diff --git a/gcc/testsuite/gcc.dg/ipa/propalign-5.c b/gcc/testsuite/gcc.dg/ipa/propalign-5.c index f2cf600..68e57da 100644 --- a/gcc/testsuite/gcc.dg/ipa/propalign-5.c +++ b/gcc/testsuite/gcc.dg/ipa/propalign-5.c @@ -20,4 +20,4 @@ main() test (&bb); return 0; } -/* { dg-final { scan-ipa-dump "Alignment 2" "cp" } } */ +/* { dg-final { scan-ipa-dump "align: 2" "cp" } } */