Hi, Martin, Thanks a lot for your experiments and examples, they are really helpful.
So, based on your study, I will delete the code that handle grp_to_be_debug_replaced accesses in generate_subtree_deferred_init. Let me know if you have further comments on this. Qing > On Jul 12, 2021, at 12:06 PM, Martin Jambor <mjam...@suse.cz> wrote: > > Hi, > > On Mon, Jul 12 2021, Qing Zhao wrote: >>> 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 very flattered that you think I understand debug_binds well :-) (I > don't) > >> >> 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)? >> > > But I have done some simple experiments and reached the conclusion the > code is never executed and wrong. It can get executed if you change the > if statement in build_access_from_expr like I explained in my previous > email: > > static bool > build_access_from_expr (tree expr, gimple *stmt, bool write) > { > struct access *access; > > access = build_access_from_expr_1 (expr, stmt, write); > if (access) > { > /* This means the aggregate is accesses as a whole in a way other than > an > assign statement and thus cannot be removed even if we had a scalar > replacement for everything. */ > if (cannot_scalarize_away_bitmap > && !gimple_call_internal_p (stmt, IFN_DEFERRED_INIT)) > bitmap_set_bit (cannot_scalarize_away_bitmap, DECL_UID > (access->base)); > return true; > } > return false; > } > > But then it ICEs when the operand scanner attempts to grok the debug bind: > > test.c:43:1: internal compiler error: in get_expr_operands, at > tree-ssa-operands.c:945 > 43 | } > > Gdb reveals the statement is what I suspected: > > #2 0x000000000156eea6 in operands_scanner::parse_ssa_operands > (this=0x7fffffffdad0) > at /home/mjambor/gcc/mine/src/gcc/tree-ssa-operands.c:973 > 973 get_expr_operands (gimple_debug_bind_get_value_ptr (stmt), > (gdb) pgg stmt > # DEBUG s2D.1971 => .DEFERRED_INIT (4, 1, 0) > > It turns out debug binds cannot have CALL_EXPRs as operands. > > So yes, just do not attempt to handle grp_to_be_debug_replaced accesses > in generate_subtree_deferred_init. In fact, if you do that (+ the > modification described above) the results seem to be rather good on the > attached testcase (which I derived from gcc.dg/guality/pr59776.c), at > least on GIMPLE and at least when compiled with -O1 -g. I did not > actually attempt to look at the generated dwarf. I have made a simple > attempt to print uninitialized (and unused) SRAed structure in gdb with > and without -ftrivial-auto-var-init=pattern, but in both cases it just > said it was optimized out. > > Martin > > #include "nop.h" > > struct S { float f, g; }; > > __attribute__((noipa)) void > foo (struct S *p, int flag) > { > struct S s1, s2; > if (flag) > { > s1 = *p; > s2 = s1; > } > else > { > s2 = s1; > } > > *(int *) &s2.f = 0; > asm volatile (NOP : : : "memory"); > asm volatile (NOP : : : "memory"); > s2 = s1; > asm volatile (NOP : : : "memory"); > asm volatile (NOP : : : "memory"); > } > > int __attribute__((noipa)) > getint(void) > { > return 1; > } > > int > main () > { > struct S x = { 5.0f, 6.0f }; > foo (&x, getint()); > return 0; > }