Replace more DEBUG_EXPR_DECL creations with build_debug_expr_decl
Commit Message
Hi,
On Tue, Nov 09 2021, Richard Biener wrote:
> On Mon, 8 Nov 2021, Martin Jambor wrote:
>> this patch introduces a helper function build_debug_expr_decl to build
>> DEBUG_EXPR_DECL tree nodes in the most common way and replaces with a
>> call of this function all code pieces which build such a DECL itself
>> and sets its mode to the TYPE_MODE of its type.
>>
>> There still remain 11 instances of open-coded creation of a
>> DEBUG_EXPR_DECL which set the mode of the DECL to something else. It
>> would probably be a good idea to figure out that has any effect and if
>> not, convert them to calls of build_debug_expr_decl too. But this
>> patch deliberately does not introduce any functional changes.
>>
>> Bootstrapped and tested on x86_64-linux, OK for trunk?
>
> OK (the const_tree suggestion is a good one).
>
> For the remaining cases I'd simply use
>
> decl = build_debug_expr_decl (type);
> SET_DECL_MODE (decl) = ...;
>
> and thus override the mode afterwards, maybe adding a comment to
> check whether that's necessary. As said, the only case where it
> might matter is when we create a debug decl replacement for a FIELD_DECL,
> so maybe for those SRA things we create for DWARF "piece" info?
>
Like this? This patch replaces all but one remaining open coded
constructions of DEBUG_EXPR_DECL with calls to build_debug_expr_decl,
even if - in order not to introduce any functional change - the mode of
the constructed decl is then overwritten.
It is not clear if changing the mode has any effect in practice and
therefore I have added a FIXME note to code which does it, as
requested.
After this patch, DEBUG_EXPR_DECLs are created only by
build_debug_expr_decl and make_debug_expr_from_rtl which looks like
it should be left alone.
Bootstrapped and tested on x86_64-linux. OK for trunk?
I have also compared the generated DWARF (with readelf -w) of cc1plus
generated by a compiler with this patch and one with the mode setting
removed (on top of this patch) and there were no differences
whatsoever. So perhaps we can just remove it? I have not
bootstrapped that patch yet, though.
Thanks,
Martin
gcc/ChangeLog:
2021-11-11 Martin Jambor <mjambor@suse.cz>
* cfgexpand.c (expand_gimple_basic_block): Use build_debug_expr_decl,
add a fixme note about the mode assignment perhaps being unnecessary.
* ipa-param-manipulation.c (ipa_param_adjustments::modify_call):
Likewise.
(ipa_param_body_adjustments::mark_dead_statements): Likewise.
(ipa_param_body_adjustments::reset_debug_stmts): Likewise.
* tree-inline.c (remap_ssa_name): Likewise.
(tree_function_versioning): Likewise.
* tree-into-ssa.c (rewrite_debug_stmt_uses): Likewise.
* tree-ssa-loop-ivopts.c (remove_unused_ivs): Likewise.
* tree-ssa.c (insert_debug_temp_for_var_def): Likewise.
---
gcc/cfgexpand.c | 5 ++---
gcc/ipa-param-manipulation.c | 17 +++++++----------
gcc/tree-inline.c | 17 +++++++----------
gcc/tree-into-ssa.c | 7 +++----
gcc/tree-ssa-loop-ivopts.c | 5 ++---
gcc/tree-ssa.c | 5 ++---
6 files changed, 23 insertions(+), 33 deletions(-)
Comments
On November 12, 2021 3:39:56 PM GMT+01:00, Martin Jambor <mjambor@suse.cz> wrote:
>Hi,
>
>On Tue, Nov 09 2021, Richard Biener wrote:
>> On Mon, 8 Nov 2021, Martin Jambor wrote:
>>> this patch introduces a helper function build_debug_expr_decl to build
>>> DEBUG_EXPR_DECL tree nodes in the most common way and replaces with a
>>> call of this function all code pieces which build such a DECL itself
>>> and sets its mode to the TYPE_MODE of its type.
>>>
>>> There still remain 11 instances of open-coded creation of a
>>> DEBUG_EXPR_DECL which set the mode of the DECL to something else. It
>>> would probably be a good idea to figure out that has any effect and if
>>> not, convert them to calls of build_debug_expr_decl too. But this
>>> patch deliberately does not introduce any functional changes.
>>>
>>> Bootstrapped and tested on x86_64-linux, OK for trunk?
>>
>> OK (the const_tree suggestion is a good one).
>>
>> For the remaining cases I'd simply use
>>
>> decl = build_debug_expr_decl (type);
>> SET_DECL_MODE (decl) = ...;
>>
>> and thus override the mode afterwards, maybe adding a comment to
>> check whether that's necessary. As said, the only case where it
>> might matter is when we create a debug decl replacement for a FIELD_DECL,
>> so maybe for those SRA things we create for DWARF "piece" info?
>>
>
>Like this? This patch replaces all but one remaining open coded
>constructions of DEBUG_EXPR_DECL with calls to build_debug_expr_decl,
>even if - in order not to introduce any functional change - the mode of
>the constructed decl is then overwritten.
>
>It is not clear if changing the mode has any effect in practice and
>therefore I have added a FIXME note to code which does it, as
>requested.
>
>After this patch, DEBUG_EXPR_DECLs are created only by
>build_debug_expr_decl and make_debug_expr_from_rtl which looks like
>it should be left alone.
>
>Bootstrapped and tested on x86_64-linux. OK for trunk?
Yes.
Thanks,
Richard.
>I have also compared the generated DWARF (with readelf -w) of cc1plus
>generated by a compiler with this patch and one with the mode setting
>removed (on top of this patch) and there were no differences
>whatsoever. So perhaps we can just remove it? I have not
>bootstrapped that patch yet, though.
I guess that for one case it mattered and we might have a testcase to show that and other cases were just cut and pasted from the "wrong" place...
Richard.
>Thanks,
>
>Martin
>
>
>gcc/ChangeLog:
>
>2021-11-11 Martin Jambor <mjambor@suse.cz>
>
> * cfgexpand.c (expand_gimple_basic_block): Use build_debug_expr_decl,
> add a fixme note about the mode assignment perhaps being unnecessary.
> * ipa-param-manipulation.c (ipa_param_adjustments::modify_call):
> Likewise.
> (ipa_param_body_adjustments::mark_dead_statements): Likewise.
> (ipa_param_body_adjustments::reset_debug_stmts): Likewise.
> * tree-inline.c (remap_ssa_name): Likewise.
> (tree_function_versioning): Likewise.
> * tree-into-ssa.c (rewrite_debug_stmt_uses): Likewise.
> * tree-ssa-loop-ivopts.c (remove_unused_ivs): Likewise.
> * tree-ssa.c (insert_debug_temp_for_var_def): Likewise.
>---
> gcc/cfgexpand.c | 5 ++---
> gcc/ipa-param-manipulation.c | 17 +++++++----------
> gcc/tree-inline.c | 17 +++++++----------
> gcc/tree-into-ssa.c | 7 +++----
> gcc/tree-ssa-loop-ivopts.c | 5 ++---
> gcc/tree-ssa.c | 5 ++---
> 6 files changed, 23 insertions(+), 33 deletions(-)
>
>diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>index 55ff75bd78e..eb6466f4be6 100644
>--- a/gcc/cfgexpand.c
>+++ b/gcc/cfgexpand.c
>@@ -5898,18 +5898,17 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
> temporary. */
> gimple *debugstmt;
> tree value = gimple_assign_rhs_to_tree (def);
>- tree vexpr = make_node (DEBUG_EXPR_DECL);
>+ tree vexpr = build_debug_expr_decl (TREE_TYPE (value));
> rtx val;
> machine_mode mode;
>
> set_curr_insn_location (gimple_location (def));
>
>- DECL_ARTIFICIAL (vexpr) = 1;
>- TREE_TYPE (vexpr) = TREE_TYPE (value);
> if (DECL_P (value))
> mode = DECL_MODE (value);
> else
> mode = TYPE_MODE (TREE_TYPE (value));
>+ /* FIXME: Is setting the mode really necessary? */
> SET_DECL_MODE (vexpr, mode);
>
> val = gen_rtx_VAR_LOCATION
>diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
>index ae3149718ca..a230735d71e 100644
>--- a/gcc/ipa-param-manipulation.c
>+++ b/gcc/ipa-param-manipulation.c
>@@ -831,9 +831,8 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs,
> }
> if (ddecl == NULL)
> {
>- ddecl = make_node (DEBUG_EXPR_DECL);
>- DECL_ARTIFICIAL (ddecl) = 1;
>- TREE_TYPE (ddecl) = TREE_TYPE (origin);
>+ ddecl = build_debug_expr_decl (TREE_TYPE (origin));
>+ /* FIXME: Is setting the mode really necessary? */
> SET_DECL_MODE (ddecl, DECL_MODE (origin));
>
> vec_safe_push (*debug_args, origin);
>@@ -1063,9 +1062,8 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param,
> return;
> }
>
>- tree dp_ddecl = make_node (DEBUG_EXPR_DECL);
>- DECL_ARTIFICIAL (dp_ddecl) = 1;
>- TREE_TYPE (dp_ddecl) = TREE_TYPE (dead_param);
>+ tree dp_ddecl = build_debug_expr_decl (TREE_TYPE (dead_param));
>+ /* FIXME: Is setting the mode really necessary? */
> SET_DECL_MODE (dp_ddecl, DECL_MODE (dead_param));
> m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl);
> }
>@@ -2217,11 +2215,10 @@ ipa_param_body_adjustments::reset_debug_stmts ()
> gcc_assert (is_gimple_debug (stmt));
> if (vexpr == NULL && gsip != NULL)
> {
>- vexpr = make_node (DEBUG_EXPR_DECL);
>- def_temp = gimple_build_debug_source_bind (vexpr, decl, NULL);
>- DECL_ARTIFICIAL (vexpr) = 1;
>- TREE_TYPE (vexpr) = TREE_TYPE (name);
>+ vexpr = build_debug_expr_decl (TREE_TYPE (name));
>+ /* FIXME: Is setting the mode really necessary? */
> SET_DECL_MODE (vexpr, DECL_MODE (decl));
>+ def_temp = gimple_build_debug_source_bind (vexpr, decl, NULL);
> gsi_insert_before (gsip, def_temp, GSI_SAME_STMT);
> }
> if (vexpr)
>diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
>index 53d664ec2e4..8c108d8e4e7 100644
>--- a/gcc/tree-inline.c
>+++ b/gcc/tree-inline.c
>@@ -193,7 +193,6 @@ remap_ssa_name (tree name, copy_body_data *id)
> && id->entry_bb == NULL
> && single_succ_p (ENTRY_BLOCK_PTR_FOR_FN (cfun)))
> {
>- tree vexpr = make_node (DEBUG_EXPR_DECL);
> gimple *def_temp;
> gimple_stmt_iterator gsi;
> tree val = SSA_NAME_VAR (name);
>@@ -210,10 +209,10 @@ remap_ssa_name (tree name, copy_body_data *id)
> n = id->decl_map->get (val);
> if (n && TREE_CODE (*n) == DEBUG_EXPR_DECL)
> return *n;
>- def_temp = gimple_build_debug_source_bind (vexpr, val, NULL);
>- DECL_ARTIFICIAL (vexpr) = 1;
>- TREE_TYPE (vexpr) = TREE_TYPE (name);
>+ tree vexpr = build_debug_expr_decl (TREE_TYPE (name));
>+ /* FIXME: Is setting the mode really necessary? */
> SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name)));
>+ def_temp = gimple_build_debug_source_bind (vexpr, val, NULL);
> gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
> gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);
> insert_decl_map (id, val, vexpr);
>@@ -6450,9 +6449,8 @@ tree_function_versioning (tree old_decl, tree new_decl,
> debug_args = decl_debug_args_insert (new_decl);
> len = vec_safe_length (*debug_args);
> }
>- ddecl = make_node (DEBUG_EXPR_DECL);
>- DECL_ARTIFICIAL (ddecl) = 1;
>- TREE_TYPE (ddecl) = TREE_TYPE (parm);
>+ ddecl = build_debug_expr_decl (TREE_TYPE (parm));
>+ /* FIXME: Is setting the mode really necessary? */
> SET_DECL_MODE (ddecl, DECL_MODE (parm));
> vec_safe_push (*debug_args, DECL_ORIGIN (parm));
> vec_safe_push (*debug_args, ddecl);
>@@ -6488,9 +6486,8 @@ tree_function_versioning (tree old_decl, tree new_decl,
> vexpr = *d;
> if (!vexpr)
> {
>- vexpr = make_node (DEBUG_EXPR_DECL);
>- DECL_ARTIFICIAL (vexpr) = 1;
>- TREE_TYPE (vexpr) = TREE_TYPE (parm);
>+ vexpr = build_debug_expr_decl (TREE_TYPE (parm));
>+ /* FIXME: Is setting the mode really necessary? */
> SET_DECL_MODE (vexpr, DECL_MODE (parm));
> }
> def_temp = gimple_build_debug_bind (var, vexpr, NULL);
>diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c
>index 8045e34df26..265dcc5d42f 100644
>--- a/gcc/tree-into-ssa.c
>+++ b/gcc/tree-into-ssa.c
>@@ -1284,11 +1284,10 @@ rewrite_debug_stmt_uses (gimple *stmt)
> if (def == NULL_TREE)
> {
> gimple *def_temp;
>- def = make_node (DEBUG_EXPR_DECL);
>- def_temp = gimple_build_debug_source_bind (def, var, NULL);
>- DECL_ARTIFICIAL (def) = 1;
>- TREE_TYPE (def) = TREE_TYPE (var);
>+ def = build_debug_expr_decl (TREE_TYPE (var));
>+ /* FIXME: Is setting the mode really necessary? */
> SET_DECL_MODE (def, DECL_MODE (var));
>+ def_temp = gimple_build_debug_source_bind (def, var, NULL);
> gsi =
> gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
> gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);
>diff --git a/gcc/tree-ssa-loop-ivopts.c b/gcc/tree-ssa-loop-ivopts.c
>index 4a498abe3b0..5a7fd305d91 100644
>--- a/gcc/tree-ssa-loop-ivopts.c
>+++ b/gcc/tree-ssa-loop-ivopts.c
>@@ -7742,9 +7742,8 @@ remove_unused_ivs (struct ivopts_data *data, bitmap toremove)
> comp = unshare_expr (comp);
> if (count > 1)
> {
>- tree vexpr = make_node (DEBUG_EXPR_DECL);
>- DECL_ARTIFICIAL (vexpr) = 1;
>- TREE_TYPE (vexpr) = TREE_TYPE (comp);
>+ tree vexpr = build_debug_expr_decl (TREE_TYPE (comp));
>+ /* FIXME: Is setting the mode really necessary? */
> if (SSA_NAME_VAR (def))
> SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (def)));
> else
>diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c
>index 3f25d654d3f..1565e21d983 100644
>--- a/gcc/tree-ssa.c
>+++ b/gcc/tree-ssa.c
>@@ -434,14 +434,13 @@ insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
> else
> {
> gdebug *def_temp;
>- tree vexpr = make_node (DEBUG_EXPR_DECL);
>+ tree vexpr = build_debug_expr_decl (TREE_TYPE (value));
>
> def_temp = gimple_build_debug_bind (vexpr,
> unshare_expr (value),
> def_stmt);
>
>- DECL_ARTIFICIAL (vexpr) = 1;
>- TREE_TYPE (vexpr) = TREE_TYPE (value);
>+ /* FIXME: Is setting the mode really necessary? */
> if (DECL_P (value))
> SET_DECL_MODE (vexpr, DECL_MODE (value));
> else
@@ -5898,18 +5898,17 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
temporary. */
gimple *debugstmt;
tree value = gimple_assign_rhs_to_tree (def);
- tree vexpr = make_node (DEBUG_EXPR_DECL);
+ tree vexpr = build_debug_expr_decl (TREE_TYPE (value));
rtx val;
machine_mode mode;
set_curr_insn_location (gimple_location (def));
- DECL_ARTIFICIAL (vexpr) = 1;
- TREE_TYPE (vexpr) = TREE_TYPE (value);
if (DECL_P (value))
mode = DECL_MODE (value);
else
mode = TYPE_MODE (TREE_TYPE (value));
+ /* FIXME: Is setting the mode really necessary? */
SET_DECL_MODE (vexpr, mode);
val = gen_rtx_VAR_LOCATION
@@ -831,9 +831,8 @@ ipa_param_adjustments::modify_call (cgraph_edge *cs,
}
if (ddecl == NULL)
{
- ddecl = make_node (DEBUG_EXPR_DECL);
- DECL_ARTIFICIAL (ddecl) = 1;
- TREE_TYPE (ddecl) = TREE_TYPE (origin);
+ ddecl = build_debug_expr_decl (TREE_TYPE (origin));
+ /* FIXME: Is setting the mode really necessary? */
SET_DECL_MODE (ddecl, DECL_MODE (origin));
vec_safe_push (*debug_args, origin);
@@ -1063,9 +1062,8 @@ ipa_param_body_adjustments::mark_dead_statements (tree dead_param,
return;
}
- tree dp_ddecl = make_node (DEBUG_EXPR_DECL);
- DECL_ARTIFICIAL (dp_ddecl) = 1;
- TREE_TYPE (dp_ddecl) = TREE_TYPE (dead_param);
+ tree dp_ddecl = build_debug_expr_decl (TREE_TYPE (dead_param));
+ /* FIXME: Is setting the mode really necessary? */
SET_DECL_MODE (dp_ddecl, DECL_MODE (dead_param));
m_dead_ssa_debug_equiv.put (parm_ddef, dp_ddecl);
}
@@ -2217,11 +2215,10 @@ ipa_param_body_adjustments::reset_debug_stmts ()
gcc_assert (is_gimple_debug (stmt));
if (vexpr == NULL && gsip != NULL)
{
- vexpr = make_node (DEBUG_EXPR_DECL);
- def_temp = gimple_build_debug_source_bind (vexpr, decl, NULL);
- DECL_ARTIFICIAL (vexpr) = 1;
- TREE_TYPE (vexpr) = TREE_TYPE (name);
+ vexpr = build_debug_expr_decl (TREE_TYPE (name));
+ /* FIXME: Is setting the mode really necessary? */
SET_DECL_MODE (vexpr, DECL_MODE (decl));
+ def_temp = gimple_build_debug_source_bind (vexpr, decl, NULL);
gsi_insert_before (gsip, def_temp, GSI_SAME_STMT);
}
if (vexpr)
@@ -193,7 +193,6 @@ remap_ssa_name (tree name, copy_body_data *id)
&& id->entry_bb == NULL
&& single_succ_p (ENTRY_BLOCK_PTR_FOR_FN (cfun)))
{
- tree vexpr = make_node (DEBUG_EXPR_DECL);
gimple *def_temp;
gimple_stmt_iterator gsi;
tree val = SSA_NAME_VAR (name);
@@ -210,10 +209,10 @@ remap_ssa_name (tree name, copy_body_data *id)
n = id->decl_map->get (val);
if (n && TREE_CODE (*n) == DEBUG_EXPR_DECL)
return *n;
- def_temp = gimple_build_debug_source_bind (vexpr, val, NULL);
- DECL_ARTIFICIAL (vexpr) = 1;
- TREE_TYPE (vexpr) = TREE_TYPE (name);
+ tree vexpr = build_debug_expr_decl (TREE_TYPE (name));
+ /* FIXME: Is setting the mode really necessary? */
SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name)));
+ def_temp = gimple_build_debug_source_bind (vexpr, val, NULL);
gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);
insert_decl_map (id, val, vexpr);
@@ -6450,9 +6449,8 @@ tree_function_versioning (tree old_decl, tree new_decl,
debug_args = decl_debug_args_insert (new_decl);
len = vec_safe_length (*debug_args);
}
- ddecl = make_node (DEBUG_EXPR_DECL);
- DECL_ARTIFICIAL (ddecl) = 1;
- TREE_TYPE (ddecl) = TREE_TYPE (parm);
+ ddecl = build_debug_expr_decl (TREE_TYPE (parm));
+ /* FIXME: Is setting the mode really necessary? */
SET_DECL_MODE (ddecl, DECL_MODE (parm));
vec_safe_push (*debug_args, DECL_ORIGIN (parm));
vec_safe_push (*debug_args, ddecl);
@@ -6488,9 +6486,8 @@ tree_function_versioning (tree old_decl, tree new_decl,
vexpr = *d;
if (!vexpr)
{
- vexpr = make_node (DEBUG_EXPR_DECL);
- DECL_ARTIFICIAL (vexpr) = 1;
- TREE_TYPE (vexpr) = TREE_TYPE (parm);
+ vexpr = build_debug_expr_decl (TREE_TYPE (parm));
+ /* FIXME: Is setting the mode really necessary? */
SET_DECL_MODE (vexpr, DECL_MODE (parm));
}
def_temp = gimple_build_debug_bind (var, vexpr, NULL);
@@ -1284,11 +1284,10 @@ rewrite_debug_stmt_uses (gimple *stmt)
if (def == NULL_TREE)
{
gimple *def_temp;
- def = make_node (DEBUG_EXPR_DECL);
- def_temp = gimple_build_debug_source_bind (def, var, NULL);
- DECL_ARTIFICIAL (def) = 1;
- TREE_TYPE (def) = TREE_TYPE (var);
+ def = build_debug_expr_decl (TREE_TYPE (var));
+ /* FIXME: Is setting the mode really necessary? */
SET_DECL_MODE (def, DECL_MODE (var));
+ def_temp = gimple_build_debug_source_bind (def, var, NULL);
gsi =
gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);
@@ -7742,9 +7742,8 @@ remove_unused_ivs (struct ivopts_data *data, bitmap toremove)
comp = unshare_expr (comp);
if (count > 1)
{
- tree vexpr = make_node (DEBUG_EXPR_DECL);
- DECL_ARTIFICIAL (vexpr) = 1;
- TREE_TYPE (vexpr) = TREE_TYPE (comp);
+ tree vexpr = build_debug_expr_decl (TREE_TYPE (comp));
+ /* FIXME: Is setting the mode really necessary? */
if (SSA_NAME_VAR (def))
SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (def)));
else
@@ -434,14 +434,13 @@ insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
else
{
gdebug *def_temp;
- tree vexpr = make_node (DEBUG_EXPR_DECL);
+ tree vexpr = build_debug_expr_decl (TREE_TYPE (value));
def_temp = gimple_build_debug_bind (vexpr,
unshare_expr (value),
def_stmt);
- DECL_ARTIFICIAL (vexpr) = 1;
- TREE_TYPE (vexpr) = TREE_TYPE (value);
+ /* FIXME: Is setting the mode really necessary? */
if (DECL_P (value))
SET_DECL_MODE (vexpr, DECL_MODE (value));
else