Re: [PATCH 7/9] ENABLE_CHECKING refactoring: middle-end, LTO FE

[Bernd Schmidt <bschmidt@redhat.com>]

> diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
> -
> -#ifdef ENABLE_CHECKING
> -              verify_flow_info ();
> -#endif
> +	      checking_verify_flow_info ();

This looks misindented.

> -#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);

> -#ifdef ENABLE_CHECKING
> -  if (check_same_comdat_groups)
> +  if (CHECKING_P && check_same_comdat_groups)

flag_checking

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

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

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

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

> 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, &reg_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.

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=*/.

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

>   	  tree t = (*trees)[i];
> -#ifdef ENABLE_CHECKING
> -	  if (TYPE_P (t))
> +	  if (CHECKING_P && TYPE_P (t))
>   	    verify_type (t);
> -#endif

flag_checking

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

> -#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?

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

> +  if (CHECKING_P)
> +    {
> +      for (; argno < PP_NL_ARGMAX; argno++)
> +	gcc_assert (!formatters[argno]);
> +    }

Here too. Use flag_checking.

> +  if (CHECKING_P && mode != VOIDmode)

flag_checking.

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

> -#endif
> -
>
> +

Don't change the whitespace here. Looks like you probably removed a page 
break.

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

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

> +      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.
> @@ -211,9 +211,9 @@ pack_cumulative_args (CUMULATIVE_ARGS *arg)
>   {
>     cumulative_args_t ret;
>
> -#ifdef ENABLE_CHECKING
> +#if CHECKING_P
>     ret.magic = CUMULATIVE_ARGS_MAGIC;
> -#endif /* ENABLE_CHECKING */
> +#endif /* CHECKING_P */

In this case I think it's ok to keep using conditional compilation as 
you're doing, since the extra magic field in cumulative_args_t looks 
like it would be expensive. (Not that we're spending a huge amount of 
time computing arg layout, but still...)

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

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

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


Bernd