Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Martin Liška]

On 11/19/21 15:06, Richard Biener wrote:

Can you please revert it?


Sure, done as 79e9f721d1a6f370ce0534745baeeb5a56da948e.

Martin

Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Richard Biener via Gcc-patches]

On Fri, Nov 19, 2021 at 2:22 PM Martin Liška  wrote:
>
> On 11/19/21 09:59, Alexandre Oliva wrote:
> > So I find the abstraction useful.  However, I don't have plans to add
> > other kinds of debug stmts, and I don't know of anyone else who does, so
> > I won't stand in the way if others think removing these abstractions is
> > a positive change.
>
> Hello.
>
> I've already installed the patch based on Jeff's approval (and waiting for 24 
> hours)
> as 206b22d021d94adbaa79e1d443c87415254b15de.

Can you please revert it?

> Martin

Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Martin Liška]

On 11/19/21 09:59, Alexandre Oliva wrote:

So I find the abstraction useful.  However, I don't have plans to add
other kinds of debug stmts, and I don't know of anyone else who does, so
I won't stand in the way if others think removing these abstractions is
a positive change.


Hello.

I've already installed the patch based on Jeff's approval (and waiting for 24 
hours)
as 206b22d021d94adbaa79e1d443c87415254b15de.

Martin

Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Jakub Jelinek via Gcc-patches]

On Fri, Nov 19, 2021 at 10:38:41AM +0100, Richard Biener via Gcc-patches wrote:
> > Yup.  Though there is a 1:1 equivalence right now, conceptually other
> > kinds of debug marker stmts, and of debug bind stmts, could be
> > introduced, and then the macros would be adjusted to encompass the new
> > functionality, covering presumably different options as well.
> >
> > By removing the macros, every use of the options would have to be
> > reassessed to tell whether it needs to be changed to cover the new
> > features, or left alone because it's really meant to refer to that
> > specific option.
> >
> > So I find the abstraction useful.  However, I don't have plans to add
> > other kinds of debug stmts, and I don't know of anyone else who does, so
> > I won't stand in the way if others think removing these abstractions is
> > a positive change.
> 
> Since the patch keeps some abstraction but not all I think we should
> keep them all for now.

Yeah, e.g. there has been discussions about deferring some warnings until
later so we omit less often false positives about dead code that we optimize
away, and one suggested approach was to represent those warnings as special
debug stmts.  Those would appear in the IL regardless of the
-fvariable-tracking-assignments option, so MAY_HAVE_DEBUG_STMTS could
become
  debug_nonbind_markers_p || flag_var_tracking_assignments || 
cfun->may_have_warnings
where the last one would be set if we emit any such warning stmts into the
IL.

Jakub

Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Richard Biener via Gcc-patches]

On Fri, Nov 19, 2021 at 9:59 AM Alexandre Oliva  wrote:
>
> On Oct 18, 2021, Richard Biener via Gcc-patches  
> wrote:
>
> > On Mon, Oct 18, 2021 at 10:54 AM Martin Liška  wrote:
> >>
> >> The macros correspond 1:1 to an option flags and make it harder
> >> to find all usages of the flags.
> >>
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> Ready to be installed?
>
> > Hmm, they were introduced on purpose
>
> Yup.  Though there is a 1:1 equivalence right now, conceptually other
> kinds of debug marker stmts, and of debug bind stmts, could be
> introduced, and then the macros would be adjusted to encompass the new
> functionality, covering presumably different options as well.
>
> By removing the macros, every use of the options would have to be
> reassessed to tell whether it needs to be changed to cover the new
> features, or left alone because it's really meant to refer to that
> specific option.
>
> So I find the abstraction useful.  However, I don't have plans to add
> other kinds of debug stmts, and I don't know of anyone else who does, so
> I won't stand in the way if others think removing these abstractions is
> a positive change.

Since the patch keeps some abstraction but not all I think we should
keep them all for now.

Richard.

>
> --
> Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
>Free Software Activist   GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about 

Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Alexandre Oliva via Gcc-patches]

On Oct 18, 2021, Richard Biener via Gcc-patches  wrote:

> On Mon, Oct 18, 2021 at 10:54 AM Martin Liška  wrote:
>> 
>> The macros correspond 1:1 to an option flags and make it harder
>> to find all usages of the flags.
>> 
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>> 
>> Ready to be installed?

> Hmm, they were introduced on purpose

Yup.  Though there is a 1:1 equivalence right now, conceptually other
kinds of debug marker stmts, and of debug bind stmts, could be
introduced, and then the macros would be adjusted to encompass the new
functionality, covering presumably different options as well.

By removing the macros, every use of the options would have to be
reassessed to tell whether it needs to be changed to cover the new
features, or left alone because it's really meant to refer to that
specific option.

So I find the abstraction useful.  However, I don't have plans to add
other kinds of debug stmts, and I don't know of anyone else who does, so
I won't stand in the way if others think removing these abstractions is
a positive change.

-- 
Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
   Free Software Activist   GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about 

Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Jeff Law via Gcc-patches]

On 11/12/2021 7:37 AM, Martin Liška wrote:

@Alexandre: PING

On 10/18/21 12:05, Richard Biener wrote:

On Mon, Oct 18, 2021 at 10:54 AM Martin Liška  wrote:


The macros correspond 1:1 to an option flags and make it harder
to find all usages of the flags.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?


Hmm, they were introduced on purpose - since you leave around
MAY_HAVE_DEBUG_STMTS they conceptually make the code
easier to understand.

So I'm not sure if we want this change.  CCed Alex so maybe he
can weight in.
I'd give it another 48hrs, then go ahead and commit.  My recollection is 
those were in place to allow the bulk of the work to go in independent 
of the switches that activated the SFN debugging bits.



Jeff

Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Martin Liška]

@Alexandre: PING

On 10/18/21 12:05, Richard Biener wrote:

On Mon, Oct 18, 2021 at 10:54 AM Martin Liška  wrote:


The macros correspond 1:1 to an option flags and make it harder
to find all usages of the flags.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?


Hmm, they were introduced on purpose - since you leave around
MAY_HAVE_DEBUG_STMTS they conceptually make the code
easier to understand.

So I'm not sure if we want this change.  CCed Alex so maybe he
can weight in.

Richard.


Thanks,
Martin

gcc/c-family/ChangeLog:

 * c-gimplify.c (genericize_c_loop): Use option directly.

gcc/c/ChangeLog:

 * c-parser.c (add_debug_begin_stmt): Use option directly.

gcc/ChangeLog:

 * cfgexpand.c (pass_expand::execute): Use option directly.
 * function.c (allocate_struct_function): Likewise.
 * gimple-low.c (lower_function_body): Likewise.
 (lower_stmt): Likewise.
 * gimple-ssa-backprop.c (backprop::prepare_change): Likewise.
 * ipa-param-manipulation.c (ipa_param_adjustments::modify_call): 
Likewise.
 * ipa-split.c (split_function): Likewise.
 * lto-streamer-in.c (input_function): Likewise.
 * sese.c (sese_insert_phis_for_liveouts): Likewise.
 * ssa-iterators.h (num_imm_uses): Likewise.
 * tree-cfg.c (make_blocks): Likewise.
 (gimple_merge_blocks): Likewise.
 * tree-inline.c (tree_function_versioning): Likewise.
 * tree-loop-distribution.c (generate_loops_for_partition): Likewise.
 * tree-sra.c (analyze_access_subtree): Likewise.
 * tree-ssa-dce.c (remove_dead_stmt): Likewise.
 * tree-ssa-loop-ivopts.c (remove_unused_ivs): Likewise.
 * tree-ssa-phiopt.c (spaceship_replacement): Likewise.
 * tree-ssa-reassoc.c (reassoc_remove_stmt): Likewise.
 * tree-ssa-tail-merge.c (tail_merge_optimize): Likewise.
 * tree-ssa-threadedge.c (propagate_threaded_block_debug_into): 
Likewise.
 * tree-ssa.c (gimple_replace_ssa_lhs): Likewise.
 (target_for_debug_bind): Likewise.
 (insert_debug_temp_for_var_def): Likewise.
 (insert_debug_temps_for_defs): Likewise.
 (reset_debug_uses): Likewise.
 * tree-ssanames.c (release_ssa_name_fn): Likewise.
 * tree-vect-loop-manip.c (adjust_vec_debug_stmts): Likewise.
 (adjust_debug_stmts): Likewise.
 (adjust_phi_and_debug_stmts): Likewise.
 (vect_do_peeling): Likewise.
 * tree-vect-loop.c (vect_transform_loop_stmt): Likewise.
 (vect_transform_loop): Likewise.
 * tree.h (MAY_HAVE_DEBUG_MARKER_STMTS): Remove
 (MAY_HAVE_DEBUG_BIND_STMTS): Remove.
 (MAY_HAVE_DEBUG_STMTS): Use options directly.

gcc/cp/ChangeLog:

 * parser.c (add_debug_begin_stmt): Use option directly.
---
   gcc/c-family/c-gimplify.c|  4 ++--
   gcc/c/c-parser.c |  2 +-
   gcc/cfgexpand.c  |  2 +-
   gcc/cp/parser.c  |  2 +-
   gcc/function.c   |  2 +-
   gcc/gimple-low.c |  4 ++--
   gcc/gimple-ssa-backprop.c|  2 +-
   gcc/ipa-param-manipulation.c |  2 +-
   gcc/ipa-split.c  |  6 +++---
   gcc/lto-streamer-in.c|  4 ++--
   gcc/sese.c   |  2 +-
   gcc/ssa-iterators.h  |  2 +-
   gcc/tree-cfg.c   |  4 ++--
   gcc/tree-inline.c|  2 +-
   gcc/tree-loop-distribution.c |  2 +-
   gcc/tree-sra.c   |  2 +-
   gcc/tree-ssa-dce.c   |  2 +-
   gcc/tree-ssa-loop-ivopts.c   |  2 +-
   gcc/tree-ssa-phiopt.c|  2 +-
   gcc/tree-ssa-reassoc.c   |  2 +-
   gcc/tree-ssa-tail-merge.c|  2 +-
   gcc/tree-ssa-threadedge.c|  2 +-
   gcc/tree-ssa.c   | 10 +-
   gcc/tree-ssanames.c  |  2 +-
   gcc/tree-vect-loop-manip.c   |  8 
   gcc/tree-vect-loop.c |  4 ++--
   gcc/tree.h   |  7 +--
   27 files changed, 41 insertions(+), 46 deletions(-)

diff --git a/gcc/c-family/c-gimplify.c b/gcc/c-family/c-gimplify.c
index 0d38b706f4c..d9cf051a680 100644
--- a/gcc/c-family/c-gimplify.c
+++ b/gcc/c-family/c-gimplify.c
@@ -295,7 +295,7 @@ genericize_c_loop (tree *stmt_p, location_t start_locus, 
tree cond, tree body,
 finish_bc_block (_list, bc_continue, clab);
 if (incr)
   {
-  if (MAY_HAVE_DEBUG_MARKER_STMTS && incr_locus != UNKNOWN_LOCATION)
+  if (debug_nonbind_markers_p && incr_locus != UNKNOWN_LOCATION)
 {
   tree d = build0 (DEBUG_BEGIN_STMT, void_type_node);
   SET_EXPR_LOCATION (d, expr_loc_or_loc (incr, start_locus));
@@ -305,7 +305,7 @@ genericize_c_loop (tree *stmt_p, location_t start_locus, 
tree cond, tree body,
   }
 append_to_statement_list (entry, _list);

-  if (MAY_HAVE_DEBUG_MARKER_STMTS && cond_locus != UNKNOWN_LOCATION)
+  if (debug_nonbind_markers_p && cond_locus != UNKNOWN_LOCATION)
   {
 

Re: [PATCH] Remove MAY_HAVE_DEBUG_MARKER_STMTS and MAY_HAVE_DEBUG_BIND_STMTS.

[Richard Biener via Gcc-patches]

On Mon, Oct 18, 2021 at 10:54 AM Martin Liška  wrote:
>
> The macros correspond 1:1 to an option flags and make it harder
> to find all usages of the flags.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

Hmm, they were introduced on purpose - since you leave around
MAY_HAVE_DEBUG_STMTS they conceptually make the code
easier to understand.

So I'm not sure if we want this change.  CCed Alex so maybe he
can weight in.

Richard.

> Thanks,
> Martin
>
> gcc/c-family/ChangeLog:
>
> * c-gimplify.c (genericize_c_loop): Use option directly.
>
> gcc/c/ChangeLog:
>
> * c-parser.c (add_debug_begin_stmt): Use option directly.
>
> gcc/ChangeLog:
>
> * cfgexpand.c (pass_expand::execute): Use option directly.
> * function.c (allocate_struct_function): Likewise.
> * gimple-low.c (lower_function_body): Likewise.
> (lower_stmt): Likewise.
> * gimple-ssa-backprop.c (backprop::prepare_change): Likewise.
> * ipa-param-manipulation.c (ipa_param_adjustments::modify_call): 
> Likewise.
> * ipa-split.c (split_function): Likewise.
> * lto-streamer-in.c (input_function): Likewise.
> * sese.c (sese_insert_phis_for_liveouts): Likewise.
> * ssa-iterators.h (num_imm_uses): Likewise.
> * tree-cfg.c (make_blocks): Likewise.
> (gimple_merge_blocks): Likewise.
> * tree-inline.c (tree_function_versioning): Likewise.
> * tree-loop-distribution.c (generate_loops_for_partition): Likewise.
> * tree-sra.c (analyze_access_subtree): Likewise.
> * tree-ssa-dce.c (remove_dead_stmt): Likewise.
> * tree-ssa-loop-ivopts.c (remove_unused_ivs): Likewise.
> * tree-ssa-phiopt.c (spaceship_replacement): Likewise.
> * tree-ssa-reassoc.c (reassoc_remove_stmt): Likewise.
> * tree-ssa-tail-merge.c (tail_merge_optimize): Likewise.
> * tree-ssa-threadedge.c (propagate_threaded_block_debug_into): 
> Likewise.
> * tree-ssa.c (gimple_replace_ssa_lhs): Likewise.
> (target_for_debug_bind): Likewise.
> (insert_debug_temp_for_var_def): Likewise.
> (insert_debug_temps_for_defs): Likewise.
> (reset_debug_uses): Likewise.
> * tree-ssanames.c (release_ssa_name_fn): Likewise.
> * tree-vect-loop-manip.c (adjust_vec_debug_stmts): Likewise.
> (adjust_debug_stmts): Likewise.
> (adjust_phi_and_debug_stmts): Likewise.
> (vect_do_peeling): Likewise.
> * tree-vect-loop.c (vect_transform_loop_stmt): Likewise.
> (vect_transform_loop): Likewise.
> * tree.h (MAY_HAVE_DEBUG_MARKER_STMTS): Remove
> (MAY_HAVE_DEBUG_BIND_STMTS): Remove.
> (MAY_HAVE_DEBUG_STMTS): Use options directly.
>
> gcc/cp/ChangeLog:
>
> * parser.c (add_debug_begin_stmt): Use option directly.
> ---
>   gcc/c-family/c-gimplify.c|  4 ++--
>   gcc/c/c-parser.c |  2 +-
>   gcc/cfgexpand.c  |  2 +-
>   gcc/cp/parser.c  |  2 +-
>   gcc/function.c   |  2 +-
>   gcc/gimple-low.c |  4 ++--
>   gcc/gimple-ssa-backprop.c|  2 +-
>   gcc/ipa-param-manipulation.c |  2 +-
>   gcc/ipa-split.c  |  6 +++---
>   gcc/lto-streamer-in.c|  4 ++--
>   gcc/sese.c   |  2 +-
>   gcc/ssa-iterators.h  |  2 +-
>   gcc/tree-cfg.c   |  4 ++--
>   gcc/tree-inline.c|  2 +-
>   gcc/tree-loop-distribution.c |  2 +-
>   gcc/tree-sra.c   |  2 +-
>   gcc/tree-ssa-dce.c   |  2 +-
>   gcc/tree-ssa-loop-ivopts.c   |  2 +-
>   gcc/tree-ssa-phiopt.c|  2 +-
>   gcc/tree-ssa-reassoc.c   |  2 +-
>   gcc/tree-ssa-tail-merge.c|  2 +-
>   gcc/tree-ssa-threadedge.c|  2 +-
>   gcc/tree-ssa.c   | 10 +-
>   gcc/tree-ssanames.c  |  2 +-
>   gcc/tree-vect-loop-manip.c   |  8 
>   gcc/tree-vect-loop.c |  4 ++--
>   gcc/tree.h   |  7 +--
>   27 files changed, 41 insertions(+), 46 deletions(-)
>
> diff --git a/gcc/c-family/c-gimplify.c b/gcc/c-family/c-gimplify.c
> index 0d38b706f4c..d9cf051a680 100644
> --- a/gcc/c-family/c-gimplify.c
> +++ b/gcc/c-family/c-gimplify.c
> @@ -295,7 +295,7 @@ genericize_c_loop (tree *stmt_p, location_t start_locus, 
> tree cond, tree body,
> finish_bc_block (_list, bc_continue, clab);
> if (incr)
>   {
> -  if (MAY_HAVE_DEBUG_MARKER_STMTS && incr_locus != UNKNOWN_LOCATION)
> +  if (debug_nonbind_markers_p && incr_locus != UNKNOWN_LOCATION)
> {
>   tree d = build0 (DEBUG_BEGIN_STMT, void_type_node);
>   SET_EXPR_LOCATION (d, expr_loc_or_loc (incr, start_locus));
> @@ -305,7 +305,7 @@ genericize_c_loop (tree *stmt_p, location_t start_locus, 
> tree cond, tree body,
>   }
> append_to_statement_list (entry, _list);
>
> -  if (MAY_HAVE_DEBUG_MARKER_STMTS && cond_locus != UNKNOWN_LOCATION)
> +  if