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.
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
