Re: [patch][version 4]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc

[Qing Zhao <qing.zhao@oracle.com>]

Hi, Martin,

Thank you for the review and comment.

On Jul 8, 2021, at 8:29 AM, Martin Jambor <mjambor@suse.cz<mailto:mjambor@suse.cz>> wrote:

diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index c05d22f3e8f1..35051d7c6b96 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -384,6 +384,13 @@ static struct

  /* Numbber of components created when splitting aggregate parameters.  */
  int param_reductions_created;
+
+  /* Number of deferred_init calls that are modified.  */
+  int deferred_init;
+
+  /* Number of deferred_init calls that are created by
+     generate_subtree_deferred_init.  */
+  int subtree_deferred_init;
} sra_stats;

static void
@@ -4096,6 +4103,110 @@ get_repl_default_def_ssa_name (struct access *racc, tree reg_type)
  return get_or_create_ssa_default_def (cfun, racc->replacement_decl);
}

+
+/* Generate statements to call .DEFERRED_INIT to initialize scalar replacements
+   of accesses within a subtree ACCESS; all its children, siblings and their
+   children are to be processed.
+   GSI is a statement iterator used to place the new statements.  */
+static void
+generate_subtree_deferred_init (struct access *access,
+ tree init_type,
+ tree is_vla,
+ gimple_stmt_iterator *gsi,
+ location_t loc)
+{
+  do
+    {
+      if (access->grp_to_be_replaced)
+ {
+  tree repl = get_access_replacement (access);
+  gimple *call
+    = gimple_build_call_internal (IFN_DEFERRED_INIT, 3,
+  TYPE_SIZE_UNIT (TREE_TYPE (repl)),
+  init_type, is_vla);
+  gimple_call_set_lhs (call, repl);
+  gsi_insert_before (gsi, call, GSI_SAME_STMT);
+  update_stmt (call);
+  gimple_set_location (call, loc);
+  sra_stats.subtree_deferred_init++;
+ }
+      else if (access->grp_to_be_debug_replaced)
+ {
+  tree drepl = get_access_replacement (access);
+  tree call = build_call_expr_internal_loc
+     (UNKNOWN_LOCATION, IFN_DEFERRED_INIT,
+      TREE_TYPE (drepl), 3,
+      TYPE_SIZE_UNIT (TREE_TYPE (drepl)),
+      init_type, is_vla);
+  gdebug *ds = gimple_build_debug_bind (drepl, call,
+ gsi_stmt (*gsi));
+  gsi_insert_before (gsi, ds, GSI_SAME_STMT);

Is handling of grp_to_be_debug_replaced accesses necessary here?  If so,
why?  grp_to_be_debug_replaced accesses are there only to facilitate
debug information about a part of an aggregate decl is that is likely
going to be entirely removed - so that debuggers can sometimes show to
users information about what they would contain had they not removed.
It seems strange you need to mark them as uninitialized because they
should not have any consumers.  (But perhaps it is also harmless.)

This part has been discussed during the 2nd version of the patch, but I think that more discussion might be necessary.

In the previous discussion, Richard Sandiford mentioned: (https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568620.html):

=====

I guess the thing we need to decide here is whether -ftrivial-auto-var-init
should affect debug-only constructs too.  If it doesn't, exmaining removed
components in a debugger might show uninitialised values in cases where
the user was expecting initialised ones.  There would be no security
concern, but it might be surprising.

I think in principle the DRHS can contain a call to DEFERRED_INIT.
Doing that would probably require further handling elsewhere though.

=====

I am still not very confident now for this part of the change.

My questions:

1. If we don’t handle grp_to_be_debug_replaced at all, what will happen?  ( the user of the debugger will see uninitialized values in
the removed part of the aggregate?  Or something else?)
2. On the other hand, if we handle grp_to_be_debug_replaced as the current patch, what will the user of the debugger see?



On a related note, if the intent of the feature is for optimizers to
behave (almost?) as if it was not taking place,

What’s you mean by “it” here?
I believe you need to
handle specially, and probably just ignore, calls to IFN_DEFERRED_INIT
in scan_function in tree-sra.c.

Do you mean to let tree-sra phase ignore IFN_DEFERRED_INIT calls completely?

My major purpose of change tree-sra.c phase is:

Change:

tmp = .DEFERRED_INIT (24, 2, 0)

To

tmp1 = .DEFERRED_INIT (8, 2, 0);
tmp2 = .DEFERRED_INIT (8, 2, 0);
tmp3 = .DEFERRED_INIT (8, 2, 0);

Doing this is to reduce the stack usage.

 Otherwise the generated SRA access
structures will have extra write flags turned on in them and that will
lead to different behavior of the pass.

Could you please explain this more?

thanks.

Qing

Martin



+ }
+      if (access->first_child)
+ generate_subtree_deferred_init (access->first_child, init_type,
+ is_vla, gsi, loc);
+
+      access = access ->next_sibling;
+    }
+  while (access);
+}
+
+/* For a call to .DEFERRED_INIT:
+   var = .DEFERRED_INIT (size_of_var, init_type, is_vla);
+   examine the LHS variable VAR and replace it with a scalar replacement if
+   there is one, also replace the RHS call to a call to .DEFERRED_INIT of
+   the corresponding scalar relacement variable.  Examine the subtree and
+   do the scalar replacements in the subtree too.  STMT is the call, GSI is
+   the statment iterator to place newly created statement.  */
+
+static enum assignment_mod_result
+sra_modify_deferred_init (gimple *stmt, gimple_stmt_iterator *gsi)
+{
+  tree lhs = gimple_call_lhs (stmt);
+  tree init_type = gimple_call_arg (stmt, 1);
+  tree is_vla = gimple_call_arg (stmt, 2);
+
+  struct access *lhs_access = get_access_for_expr (lhs);
+  if (!lhs_access)
+    return SRA_AM_NONE;
+
+  location_t loc = gimple_location (stmt);
+
+  if (lhs_access->grp_to_be_replaced)
+    {
+      tree lhs_repl = get_access_replacement (lhs_access);
+      gimple_call_set_lhs (stmt, lhs_repl);
+      tree arg0_repl = TYPE_SIZE_UNIT (TREE_TYPE (lhs_repl));
+      gimple_call_set_arg (stmt, 0, arg0_repl);
+      sra_stats.deferred_init++;
+    }
+  else if (lhs_access->grp_to_be_debug_replaced)
+    {
+      tree lhs_drepl = get_access_replacement (lhs_access);
+      tree call = build_call_expr_internal_loc
+  (UNKNOWN_LOCATION, IFN_DEFERRED_INIT,
+  TREE_TYPE (lhs_drepl), 3,
+  TYPE_SIZE_UNIT (TREE_TYPE (lhs_drepl)),
+  init_type, is_vla);
+      gdebug *ds = gimple_build_debug_bind (lhs_drepl, call,
+    gsi_stmt (*gsi));
+      gsi_insert_before (gsi, ds, GSI_SAME_STMT);
+    }
+
+  if (lhs_access->first_child)
+    generate_subtree_deferred_init (lhs_access->first_child,
+    init_type, is_vla, gsi, loc);
+  if (lhs_access->grp_covered)
+    {
+      unlink_stmt_vdef (stmt);
+      gsi_remove (gsi, true);
+      release_defs (stmt);
+      return SRA_AM_REMOVED;
+    }
+
+  return SRA_AM_MODIFIED;
+}
+
/* Examine both sides of the assignment statement pointed to by STMT, replace
   them with a scalare replacement if there is one and generate copying of
   replacements if scalarized aggregates have been used in the assignment.  GSI
@@ -4460,17 +4571,27 @@ sra_modify_function_body (void)
     break;

   case GIMPLE_CALL:
-      /* Operands must be processed before the lhs.  */
-      for (i = 0; i < gimple_call_num_args (stmt); i++)
+      /* Handle calls to .DEFERRED_INIT specially.  */
+      if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
{
-  t = gimple_call_arg_ptr (stmt, i);
-  modified |= sra_modify_expr (t, &gsi, false);
+  assign_result = sra_modify_deferred_init (stmt, &gsi);
+  modified |= assign_result == SRA_AM_MODIFIED;
+  deleted = assign_result == SRA_AM_REMOVED;
}
-
-      if (gimple_call_lhs (stmt))
+      else
{
-  t = gimple_call_lhs_ptr (stmt);
-  modified |= sra_modify_expr (t, &gsi, true);
+  /* Operands must be processed before the lhs.  */
+  for (i = 0; i < gimple_call_num_args (stmt); i++)
+    {
+      t = gimple_call_arg_ptr (stmt, i);
+      modified |= sra_modify_expr (t, &gsi, false);
+    }
+
+        if (gimple_call_lhs (stmt))
+    {
+      t = gimple_call_lhs_ptr (stmt);
+      modified |= sra_modify_expr (t, &gsi, true);
+    }
}
     break;