Hi, > On Jul 9, 2021, at 11:18 AM, Martin Jambor <mjam...@suse.cz> wrote: >> >>> 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.
So, my current change for access->grp_to_be_debug_replaced is good? Do I need to modify any other code in addition to this in order to let debugger work correctly? Or deleting this part of code might be simple and better? >> >> 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. I will try to come up with a small example on this. > >> 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. Okay, I will study this a little bit. > >> >>> >>> 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) Okay. > >> >>> 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. Okay. > > 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, Okay, will study this a little bit more. Might have more questions on this. Thanks. Qing > I think. > > Martin