On Mon, Jul 29, 2019 at 12:37 PM Martin Liška <mli...@suse.cz> wrote:
>
> On 7/29/19 11:59 AM, Richard Biener wrote:
> > On Sun, Jul 28, 2019 at 4:57 PM Martin Liška <mli...@suse.cz> wrote:
> >>
> >> Hi.
> >>
> >> And there's one more patch that deals with delete operator
> >> which has a second argument (size). That definition GIMPLE
> >> statement of the size must be also properly marked.
> >>
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> Ready to be installed?
> >
> > This isn't a proper fix. You can't mark a random operand as necessary
> > during elimination - this only works in very constraint cases. Here
> > it will break down if the stmt you just marked depends on another stmt
> > only used by the stmt you just marked (and thus also not necessary).
>
> Ah, I see.
>
> >
> > The fix is to propagate_necessity where you either have to excempt
> > the two-operator delete from handling
>
> We want to process also these delete operators.
>
> > or to mark its second operand
> > as necessary despite eventually deleting the call.
>
> Really marking that as necessary? Why?
>
> > You can avoid
> > this in case the allocation function used the exact same size argument.
>
> That's not the case as the operator new happens in a loop:
>
> <bb 5> :
> # iftmp.0_15 = PHI <iftmp.0_21(3), iftmp.0_20(4)>
> _23 = operator new [] (iftmp.0_15);
> _24 = _23;
> MEM[(sizetype *)_24] = _19;
> _26 = _24 + 8;
> _27 = _26;
> _2 = _19 + 18446744073709551615;
> _28 = (long int) _2;
>
> <bb 6> :
> # _12 = PHI <_27(5), _29(7)>
> # _13 = PHI <_28(5), _30(7)>
> if (_13 < 0)
> goto <bb 8>; [INV]
> else
> goto <bb 7>; [INV]
>
> Btw. is the hunk moved to propagate_necessity still wrong:
>
> diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
> index cf507fa0453..1c890889932 100644
> --- a/gcc/tree-ssa-dce.c
> +++ b/gcc/tree-ssa-dce.c
> @@ -803,7 +803,23 @@ propagate_necessity (bool aggressive)
> || DECL_FUNCTION_CODE (def_callee) ==
> BUILT_IN_MALLOC
> || DECL_FUNCTION_CODE (def_callee) ==
> BUILT_IN_CALLOC))
> || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
> - continue;
> + {
> + /* Some delete operators have 2 arguments, where
> + the second argument is size of the deallocated memory.
> */
> + if (gimple_call_num_args (stmt) == 2)
Please make sure this only matches operator delete (just make the check
we already do above also set a bool flag).
> + {
> + tree ptr = gimple_call_arg (stmt, 1);
> + if (TREE_CODE (ptr) == SSA_NAME)
you can use mark_operand_necessary (ptr) here (but please name 'ptr'
differently ;))
And note you can elide this in case the size arguments to new and delete
match. I notice the above also matches
ptr = malloc (4);
delete ptr;
not sure if intended (or harmful).
Richard.
> + {
> + gimple *def_stmt = SSA_NAME_DEF_STMT (ptr);
> + if (!gimple_nop_p (def_stmt)
> + && !gimple_plf (def_stmt, STMT_NECESSARY))
> + gimple_set_plf (stmt, STMT_NECESSARY, false);
> + }
> + }
> +
> + continue;
> + }
> }
>
> FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
>
> Thanks,
> Martin
>
> >
> > Richard.
> >
> >> Thanks,
> >> Martin
>
