On Sun, 2018-11-11 at 12:10 -0700, Martin Sebor wrote:
> On 11/10/2018 07:57 PM, David Malcolm wrote:
> > On Mon, 2018-10-22 at 16:08 +0200, Richard Biener wrote:
> > > On Mon, 22 Oct 2018, David Malcolm wrote:
> > >
> > > > On Mon, 2018-10-22 at 15:56 +0200, Richard Biener wrote:
> > > > [...snip...]
> > > >
> > > > > This is what I finally applied for the original patch after
> > > > > fixing
> > > > > the above issue.
> > > > >
> > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied.
> > > > >
> > > > > Richard.
> > > > >
> > > > > 2018-10-22 Richard Biener <rguent...@suse.de>
> > > > >
> > > > > * gimple-ssa-evrp-analyze.c
> > > > > (evrp_range_analyzer::record_ranges_from_incoming_edge):
> > > > > Be
> > > > > smarter about what ranges to use.
> > > > > * tree-vrp.c (add_assert_info): Dump here.
> > > > > (register_edge_assert_for_2): Instead of here at
> > > > > multiple
> > > > > but
> > > > > not all places.
> > > >
> > > > [...snip...]
> > > >
> > > > > Index: gcc/tree-vrp.c
> > > > > =============================================================
> > > > > ====
> > > > > ==
> > > > > --- gcc/tree-vrp.c (revision 265381)
> > > > > +++ gcc/tree-vrp.c (working copy)
> > > > > @@ -2299,6 +2299,9 @@ add_assert_info (vec<assert_info>
> > > > > &asser
> > > > > info.val = val;
> > > > > info.expr = expr;
> > > > > asserts.safe_push (info);
> > > > > + dump_printf (MSG_NOTE | MSG_PRIORITY_INTERNALS,
> > > > > + "Adding assert for %T from %T %s %T\n",
> > > > > + name, expr, op_symbol_code (comp_code), val);
> > > > > }
> > > >
> > > > I think this dump_printf call needs to be wrapped in:
> > > > if (dump_enabled_p ())
> > > > since otherwise it does non-trivial work, which is then
> > > > discarded
> > > > for
> > > > the common case where dumping is disabled.
> > > >
> > > > Alternatively, should dump_printf and dump_printf_loc test have
> > > > an
> > > > early-reject internally for that?
> > >
> > > Oh, I thought it had one - at least the "old" implementation
> > > did nothing expensive so if (dump_enabled_p ()) was just used
> > > to guard multiple printf stmts, avoiding multiple no-op calls.
> > >
> > > Did you check that all existing dump_* calls are wrapped inside
> > > a dump_enabled_p () region? If so I can properly guard the
> > > above.
> > > Otherwise I think we should restore previous expectation?
> > >
> > > Richard.
> >
> > Here's a patch to address the above.
> >
> > If called when !dump_enabled_p, the dump_* functions effectively do
> > nothing, but as of r263178 this doing "nothing" involves non-
> > trivial
> > work internally.
> >
> > I wasn't sure whether the dump_* functions should assert that
> > dump_enabled_p ()
> > is true when they're called, or if they should bail out immediately
> > for this case, so in this patch I implemented both, so that we get
> > an assertion failure, and otherwise bail out for the case where
> > !dump_enabled_p when assertions are disabled.
> >
> > Alternatively, we could remove the assertion, and simply have the
> > dump_* functions immediately bail out.
> >
> > Richard, do you have a preference?
> >
> > The patch also fixes all of the places I found during testing
> > (on x86_64-pc-linux-gnu) that call into dump_* but which
> > weren't guarded by
> > if (dump_enabled_p ())
> > The patch adds such conditionals.
> >
> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> >
> > OK for trunk?
> >
> > gcc/ChangeLog:
> > * dumpfile.c (VERIFY_DUMP_ENABLED_P): New macro.
> > (dump_gimple_stmt): Use it.
> > (dump_gimple_stmt_loc): Likewise.
> > (dump_gimple_expr): Likewise.
> > (dump_gimple_expr_loc): Likewise.
> > (dump_generic_expr): Likewise.
> > (dump_generic_expr_loc): Likewise.
> > (dump_printf): Likewise.
> > (dump_printf_loc): Likewise.
> > (dump_dec): Likewise.
> > (dump_dec): Likewise.
> > (dump_hex): Likewise.
> > (dump_symtab_node): Likewise.
> >
> > gcc/ChangeLog:
> > * gimple-loop-interchange.cc
> > (tree_loop_interchange::interchange):
> > Guard dump call with dump_enabled_p.
> > * graphite-isl-ast-to-gimple.c (graphite_regenerate_ast_isl):
> > Likewise.
> > * graphite-optimize-isl.c (optimize_isl): Likewise.
> > * graphite.c (graphite_transform_loops): Likewise.
> > * tree-loop-distribution.c (pass_loop_distribution::execute):
> > Likewise.
> > * tree-parloops.c (parallelize_loops): Likewise.
> > * tree-ssa-loop-niter.c (number_of_iterations_exit): Likewise.
> > * tree-vect-data-refs.c (vect_analyze_group_access_1):
> > Likewise.
> > (vect_prune_runtime_alias_test_list): Likewise.
> > * tree-vect-loop.c (vect_update_vf_for_slp): Likewise.
> > (vect_estimate_min_profitable_iters): Likewise.
> > * tree-vect-slp.c (vect_record_max_nunits): Likewise.
> > (vect_build_slp_tree_2): Likewise.
> > (vect_supported_load_permutation_p): Likewise.
> > (vect_slp_analyze_operations): Likewise.
> > (vect_slp_analyze_bb_1): Likewise.
> > (vect_slp_bb): Likewise.
> > * tree-vect-stmts.c (vect_analyze_stmt): Likewise.
> > * tree-vectorizer.c (try_vectorize_loop_1): Likewise.
> > (pass_slp_vectorize::execute): Likewise.
> > (increase_alignment): Likewise.
> > ---
> > gcc/dumpfile.c | 25 ++++++++++++
> > gcc/gimple-loop-interchange.cc | 2 +-
> > gcc/graphite-isl-ast-to-gimple.c | 11 ++++--
> > gcc/graphite-optimize-isl.c | 36 ++++++++++-------
> > gcc/graphite.c | 3 +-
> > gcc/tree-loop-distribution.c | 9 +++--
> > gcc/tree-parloops.c | 17 ++++----
> > gcc/tree-ssa-loop-niter.c | 2 +-
> > gcc/tree-vect-data-refs.c | 10 +++--
> > gcc/tree-vect-loop.c | 53 ++++++++++++++-----------
> > gcc/tree-vect-slp.c | 84 +++++++++++++++++++++++-----
> > ------------
> > gcc/tree-vect-stmts.c | 5 ++-
> > gcc/tree-vectorizer.c | 26 ++++++++-----
> > 13 files changed, 177 insertions(+), 106 deletions(-)
> >
> > diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> > index 09c2490..a1ab205 100644
> > --- a/gcc/dumpfile.c
> > +++ b/gcc/dumpfile.c
> > @@ -1184,6 +1184,19 @@ dump_context dump_context::s_default;
> > /* Implementation of dump_* API calls, calling into dump_context
> > member functions. */
> >
> > +/* Calls to the dump_* functions do non-trivial work, so they
> > ought
> > + to be guarded by:
> > + if (dump_enabled_p ())
> > + Assert that they are guarded, and, if assertions are disabled,
> > + bail out if the calls weren't properly guarded. */
> > +
> > +#define VERIFY_DUMP_ENABLED_P \
> > + do { \
> > + gcc_assert (dump_enabled_p ()); \
> > + if (!dump_enabled_p ()) \
> > + return; \
> > + } while (0)
>
> Since it behaves more like a function call (compound statement
> to be exact) I would suggest to make VERIFY_DUMP_ENABLED_P
> a function-like macro rather than object-like one.
FWIW it's not quite a function call: the "return" statement within it
returns from the function it's been put inside. Maybe that needs
expressing in the name of the macro? (all this depends on whether we
want to return or abort, or both. If we just want to return, I'd write
that directly, without a macro)
> That said, in my experience mixing assertions with well-defined
> code as in the above isn't the best design: it changes the contract
> of the function containing the macro between the two modes but API
> contracts should be immutable.
>
> If we view a program for which the condition in an assertion is
> false as undefined regardless of whether the assertion actually
> expands to anything giving subsequent code well-defined semantics
> isn't meaningful (there is no way to construct a well-defined test
> for it that passes with assertions either enabled and disabled).
>
> Martin
>
