> On Jul 12, 2021, at 2:51 AM, Richard Sandiford <richard.sandif...@arm.com> > wrote: > > Martin Jambor <mjam...@suse.cz> writes: >> 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 <mjam...@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. > > FTR, I don't have a strong opinion here. You know the code better > than I do, so if you think not generating debug binds is better then > let's do that.
I am okay with not generating debug binds here. Then I will just delete the part of code that guarded with if (access->grp_to_be_debug_replaced)? Martin, please confirm on this. Thanks. Qing > Thanks, > Richard