On 10/19/2015 06:13 AM, Bernd Schmidt wrote:
diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
-
-#ifdef ENABLE_CHECKING
- verify_flow_info ();
-#endif
+ checking_verify_flow_info ();
This looks misindented.
Looks that way, but it was a spaces vs tabs issue. Oh how I long for
the day when there's a commit hook that just fixes that stuff for us.
-#ifdef ENABLE_CHECKING
cgraph_edge *e;
gcc_checking_assert (
!(e = caller->get_edge (call_stmt)) || e->speculative);
-#endif
While you're here, that would look nicer as
gcc_checking_assert (!(e = caller->get_edge (call_stmt))
|| e->speculative);
Agreed & fixed.
-#ifdef ENABLE_CHECKING
- if (check_same_comdat_groups)
+ if (CHECKING_P && check_same_comdat_groups)
flag_checking
Agreed & fixed.
-#ifdef ENABLE_CHECKING
- struct df_rd_bb_info *bb_info = DF_RD_BB_INFO (g->bb);
-#endif
+ struct df_rd_bb_info *bb_info = flag_checking ? DF_RD_BB_INFO (g->bb)
+ : NULL;
I think no need to make that conditional, that's a bit too ugly.
Given that BB_INFO is only used in a checking path, I think we can just
sink the variable and its initialization into that path and drop the
conditional nonsense. Done. Fixed minor indention in the assert as well.
+ if (CHECKING_P)
+ sparseset_set_bit (active_defs_check, regno);
+ if (CHECKING_P)
+ sparseset_clear (active_defs_check);
> -#ifdef ENABLE_CHECKING
> - active_defs_check = sparseset_alloc (max_reg_num ());
> -#endif
> + if (CHECKING_P)
> + active_defs_check = sparseset_alloc (max_reg_num ());
> + if (CHECKING_P)
> + sparseset_free (active_defs_check);
flag_checking. Lots of other occurrences, I'll mention some but not all
but please fix them for consistency.
Those which were used in conditionals like above I fixed. I left the
CPP conditionals alone. Obviously a second round of conditional
compilation is going to be needed :-)
void
sem_item_optimizer::verify_classes (void)
{
-#if ENABLE_CHECKING
+ if (!flag_checking)
+ return;
+
Not entirely sure whether you want to wrap this into a
checking_verify_classes instead so that it remains easily callable by
the debugger?
I'd tend to agree. If I call a verify routine from the debugger, I
expect it to actually verify state, not to early exit because checking
was turned off entirely. I clearly missed that when initially reviewing
the ICF work.
I think pushing the conditional up to the callers should be sufficient,
which is what I've done.
+ if (flag_checking)
+ {
+ for (symtab_node *n = node->same_comdat_group;
+ n != node;
+ n = n->same_comdat_group)
+ /* If at least one of same comdat group functions is external,
+ all of them have to be, otherwise it is a front-end bug. */
+ gcc_assert (DECL_EXTERNAL (n->decl));
+ }
Unnecessary set of braces.
Agreed. But I've looked at both formattings and the one above actually
seems easier to look at visually to me. I thought it might be the
comments making the code look like a longer block, so I tried pushing in
the extra braces, but that wasn't really an improvement (to me) over
Mikhai's version.
I left it as-is. Obviously if you really want the unnecessary braces
squashed out, we can do that.
diff --git a/gcc/lra-assigns.c b/gcc/lra-assigns.c
index 2986f57..941a829 100644
--- a/gcc/lra-assigns.c
+++ b/gcc/lra-assigns.c
@@ -1591,7 +1591,7 @@ lra_assign (void)
bitmap_initialize (&all_spilled_pseudos, ®_obstack);
create_live_range_start_chains ();
setup_live_pseudos_and_spill_after_risky_transforms
(&all_spilled_pseudos);
-#ifdef ENABLE_CHECKING
+#if CHECKING_P
if (!flag_ipa_ra)
for (i = FIRST_PSEUDO_REGISTER; i < max_regno; i++)
if (lra_reg_info[i].nrefs != 0 && reg_renumber[i] >= 0
Seems inconsistent, use flag_checking and no #if? Looks like the problem
you're trying to solve is that a structure field exists only with
checking, I think that could just be made available unconditionally -
the struct is huge anyway.
Yea, more importantly, I don't like structs that change size based on
checking bits. I've made the field available and removed the
conditional assignment to the field. I can't see how this could
possibly be performance critical.
As mentioned in the other mail, I see no value changing the #ifdefs to
#ifs here or elsewhere in the patch.
- check_rtl (false);
-#endif
+ if (flag_checking)
+ check_rtl (/*final_p=*/false);
Lose the /*final_p=*/.
Fixed both occurrences.
-#ifdef ENABLE_CHECKING
+#if CHECKING_P
gcc_assert (!bitmap_bit_p (output, DECL_UID (node->decl)));
bitmap_set_bit (output, DECL_UID (node->decl));
#endif
Not entirely clear why this isn't using flag_checking.
Me neither. I think we can drop the conditional code here. Just to be
consistent with current behaviour, enclosing initialization of OUTPUT in
a flag_checking conditional.
There's some chance that we could get a "may be used uninitialized"
warning if we do that. I'll try it and see what happens in practice.
tree t = (*trees)[i];
-#ifdef ENABLE_CHECKING
- if (TYPE_P (t))
+ if (CHECKING_P && TYPE_P (t))
verify_type (t);
-#endif
flag_checking
Fixed.
@@ -14108,7 +14102,7 @@ lower_omp_target (gimple_stmt_iterator *gsi_p,
omp_context *ctx)
default:
break;
case OMP_CLAUSE_MAP:
-#ifdef ENABLE_CHECKING
+#if CHECKING_P
/* First check what we're prepared to handle in the following. */
switch (OMP_CLAUSE_MAP_KIND (c))
{
Here too...
Likewise.
-#ifdef ENABLE_CHECKING
-static void
+static void DEBUG_FUNCTION
verify_curr_properties (function *fn, void *data)
Hmm, I noticed a few cases where we lost the DEBUG_FUNCTION annotation
and was going to comment that this is one is odd - but don't we actually
want to keep DEBUG_FUNCTION annotations for the others as well so that
they don't get inlined everywhere and eliminated?
I would think so. I'll do a pass over and put them back where they got
lost.
+ if (flag_checking)
+ {
+ FOR_EACH_EDGE (e, ei, bb->preds)
+ gcc_assert (!bitmap_bit_p (tovisit, e->src->index)
+ || (e->flags & EDGE_DFS_BACK));
+ }
Unnecessary braces.
Fixed.
+ if (CHECKING_P)
+ {
+ for (; argno < PP_NL_ARGMAX; argno++)
+ gcc_assert (!formatters[argno]);
+ }
Here too. Use flag_checking.
Fixed.
+ if (CHECKING_P && mode != VOIDmode)
flag_checking.
Fixed.
-#ifdef ENABLE_CHECKING
static void
validate_value_data (struct value_data *vd)
{
+ if (!flag_checking)
+ return;
Same thought as before, it might be better to have this check in the
callers for easier use from the debugger.
Agreed & fixed.
-#endif
-
+
Don't change the whitespace here. Looks like you probably removed a page
break.
Not obvious where it got lost as there's no filename in the review
comments :-)
-#ifdef ENABLE_CHECKING
+#if CHECKING_P
/* This is initialized to the insn on which the driver stopped its
traversal. */
insn_t failed_insn;
#endif
I think here it would also be reasonable to make the field (and its
initializations) unconditional and use flag_checking for the code using it.
Agreed & done. There's an awful line break in the usage. I didn't try
to fix that.
-#ifdef ENABLE_CHECKING
- {
- edge e;
- edge_iterator ei;
+/* FIXME: (PR 67842) this check is incorrect. dominated_by_p has no
effect,
+ but changing it to gcc_assert (dominated_by_p (...)) causes
regressions,
+ e.g., gcc.dg/graphite/block-1.c. */
+#if 0
This change should probably be submitted separately, people are more
likely to see it than if it's buried in a sea of cosmetic changes.
Agreed. Removed from this changeset.
+ if (flag_checking)
+ {
+ /* last_set_in should now be all-zero. */
+ for (unsigned regno = 0; regno < max_gcse_regno; regno++)
+ gcc_assert (!last_set_in[regno]);
+ }
Braces.
I think with the comment before the loop that the extra brances are OK.
static void
-rewrite_trees (var_map map ATTRIBUTE_UNUSED)
+rewrite_trees (var_map map)
{
-#ifdef ENABLE_CHECKING
+ if (!flag_checking)
+ return;
Bit of an odd name for a function that only does verification. It was
considerably bigger in 4.2 at least, so maybe it ought to be renamed at
this point.
Yes, it changed significantly as a result of some of Matz's work.
Essentially we don't have to rewrite all the tress back into normal form
for expansion anymore.
Seems like that should be an independent patch.
- enum ref_step_type a_step;
- ok = suitable_reference_p (a->ref, &a_step);
- gcc_assert (ok && a_step == comp->comp_step);
- }
-#endif
+ enum ref_step_type a_step;
+ gcc_checking_assert (suitable_reference_p (a->ref, &a_step)
+ && a_step == comp->comp_step);
I think we're supposed to stop saying things like "enum" or "struct". If
you really want, you can go through the places you're changing and
modifying them so that they're changed only once. But it's not a
requirement.
Seems like this kind of thing is a separate cleanup. Someone (Trevor?)
was fixing up this kind of stuff. No strong opinions whether or not to
fix this instance. It'd be a step forward in C++-ification. But it'd
be inconsistent with the rest of the file.
diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
index 3e6d98c..0ed107c 100644
--- a/gcc/tree-stdarg.c
+++ b/gcc/tree-stdarg.c
@@ -1107,13 +1107,14 @@ expand_ifn_va_arg (function *fun)
if ((fun->curr_properties & PROP_gimple_lva) == 0)
expand_ifn_va_arg_1 (fun);
-#if ENABLE_CHECKING
+ if (!flag_checking)
+ return;
+
basic_block bb;
gimple_stmt_iterator i;
FOR_EACH_BB_FN (bb, fun)
for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
gcc_assert (!gimple_call_ifn_va_arg_p (gsi_stmt (i)));
-#endif
I think this would be better wrapped in an "if (flag_checking)", someone
might want to append to the function at some point.
Done.
I'm going to do a build/test cycle with the suggestions from above and
see what falls out.
Thanks for all the suggestions!
jeff
