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

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Thu, Jul 08 2021, Qing Zhao wrote:
> (Resend this email since the previous one didn’t quote, I changed one
> setting in my mail client, hopefully that can fix this issue).
>
> Hi, Martin,
>
> Thank you for the review and comment.
>
>> On Jul 8, 2021, at 8:29 AM, Martin Jambor <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.

I see.  I still tend to think that with or without the generation of
gimple_build_debug_binds, the debugger would still not display any value
for the component in question.  Without it there would be no information
about the component at a any place in code affected by this, with it the
component would be explicitely uninitialized.  But OK.

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

Well, can you try?  :-) I think the debugger would not have anything to
display.

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

I don't know.  It would be interesting to know if the generated DWARF is
different at all.

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

-ftrivial-auto-var-init (the feature)

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

I have thought about it a bit more and actually looked at the decision
making code and I realized I was wrong because initializations of
aggregates from a call, which IIUC DEFERRED_INIT is, would not result in
setting neither grp_assignment_write nor grp_scalar_write being set and
only these two affect the transformation decisions.

Moreover,  ignoring DEFERRED_INIT calls in scan_function might cause
a need for special-casing elsewhere and so it is not worth it.  Sorry.

But presence of calls to DEFERRED_INIT still might cause total
scalarization to not happen.  I believe you do not want to set a bit in
cannot_scalarize_away_bitmap in build_access_from_expr function if the
statement in question is a call to DEFERRED_INIT.  You should be able to
modify gcc/testsuite/gcc.dg/tree-ssa/sra-12.c to get a simple testcase,
I think.

Martin