| Message ID | 6c25e7d2-81eb-fb59-bd44-839c70cdebcd@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Remove also 2nd argument for unused delete operator (PR tree-optimization/91270). | expand |
On Sun, Jul 28, 2019 at 4:57 PM Martin Liška <mliska@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). The fix is to propagate_necessity where you either have to excempt the two-operator delete from handling or to mark its second operand as necessary despite eventually deleting the call. You can avoid this in case the allocation function used the exact same size argument. Richard. > Thanks, > Martin
On 7/29/19 11:59 AM, Richard Biener wrote: > On Sun, Jul 28, 2019 at 4:57 PM Martin Liška <mliska@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) + { + tree ptr = gimple_call_arg (stmt, 1); + if (TREE_CODE (ptr) == SSA_NAME) + { + 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
On Mon, Jul 29, 2019 at 12:37 PM Martin Liška <mliska@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 <mliska@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 >
Hi.
After playing with the test-case, I noticed that we don't want
to handle new/delete operators with a varying (SSA_NAME) as a second argument.
Considering following test-case:
$ cat /tmp/new.C
struct S {
S ();
~S();
};
int a = 123;
void fn1() {
S *s = new S[a];
delete[] s;
}
$ ./xg++ -B. /tmp/new.C -O2 -fdump-tree-gimple=/dev/stdout:
a.1_1 = a;
D.2341 = (sizetype) a.1_1;
if (D.2341 <= 9223372036854775800) goto <D.2342>; else goto <D.2343>;
<D.2342>:
iftmp.0 = D.2341 + 8;
goto <D.2344>;
<D.2343>:
iftmp.0 = 18446744073709551615;
<D.2344>:
D.2323 = operator new [] (iftmp.0);
MEM[(sizetype *)D.2323] = D.2341;
try
{
D.2324 = D.2323 + 8;
D.2325 = D.2324;
_2 = D.2341 + 18446744073709551615;
D.2326 = (long int) _2;
try
{
<D.2346>:
if (D.2326 < 0) goto <D.2338>; else goto <D.2347>;
<D.2347>:
S::S (D.2325);
D.2325 = D.2325 + 1;
D.2326 = D.2326 + -1;
goto <D.2346>;
<D.2338>:
}
catch
{
{
struct S * D.2336;
if (D.2324 != 0B) goto <D.2348>; else goto <D.2349>;
<D.2348>:
_3 = (sizetype) D.2326;
_4 = D.2341 - _3;
_5 = _4 + 18446744073709551615;
D.2336 = D.2324 + _5;
<D.2350>:
if (D.2336 == D.2324) goto <D.2351>; else goto <D.2352>;
<D.2352>:
D.2336 = D.2336 + 18446744073709551615;
S::~S (D.2336);
goto <D.2350>;
<D.2351>:
goto <D.2353>;
<D.2349>:
<D.2353>:
}
}
retval.2 = D.2324;
}
catch
{
if (D.2341 <= 9223372036854775800) goto <D.2355>; else goto <D.2356>;
<D.2355>:
iftmp.3 = D.2341 + 8;
goto <D.2357>;
<D.2356>:
iftmp.3 = 18446744073709551615;
<D.2357>:
operator delete [] (D.2323, iftmp.3);
}
s = D.2323 + 8;
{
struct S * D.2337;
if (s != 0B) goto <D.2358>; else goto <D.2359>;
<D.2358>:
try
{
_6 = s + 18446744073709551608;
_7 = *_6;
D.2337 = s + _7;
<D.2360>:
if (D.2337 == s) goto <D.2361>; else goto <D.2362>;
<D.2362>:
D.2337 = D.2337 + 18446744073709551615;
S::~S (D.2337);
goto <D.2360>;
<D.2361>:
}
finally
{
_8 = s + 18446744073709551608;
_9 = *_8;
_10 = _9 + 8;
_11 = s + 18446744073709551608;
operator delete [] (_11, _10);
}
goto <D.2363>;
<D.2359>:
<D.2363>:
}
}
as seen from the dump, we first make a operator new[] for a capped value
based on variable 'a'. Latter we have a loop that calls S:S and catch block contains another
loop calling S::~S. Similarly last part contains delete[] which is based on number of really
allocated memory (that lives in *D.2323).
Anyway that's not a candidate for DCE. I'm testing following patch.
Martin
On 7/30/19 9:46 AM, Martin Liška wrote:
> Anyway that's not a candidate for DCE. I'm testing following patch.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
One alternative approach can be to drop DECL_SET_IS_OPERATOR_DELETE in:
cat -n gcc/cp/decl.c | less
...
4410 deltype = cp_build_type_attribute_variant (deltype, extvisattr);
4411 deltype = build_exception_variant (deltype, empty_except_spec);
4412 opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
4413 DECL_SET_IS_OPERATOR_DELETE (opdel, true);
4414 opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
4415 DECL_SET_IS_OPERATOR_DELETE (opdel, true);
4416
4417 if (flag_sized_deallocation)
4418 {
4419 /* operator delete (void *, size_t, align_val_t); */
4420 deltype = build_function_type_list (void_type_node, ptr_type_node,
4421 size_type_node, align_type_node,
4422 NULL_TREE);
4423 deltype = cp_build_type_attribute_variant (deltype, extvisattr);
4424 deltype = build_exception_variant (deltype, empty_except_spec);
4425 opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
4426 DECL_SET_IS_OPERATOR_DELETE (opdel, true);
4427 opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
4428 DECL_SET_IS_OPERATOR_DELETE (opdel, true);
4429 }
4430 }
at lines 4426 and 4428.
Richi what do you prefer?
Martin
On Tue, Jul 30, 2019 at 10:07 AM Martin Liška <mliska@suse.cz> wrote: > > On 7/30/19 9:46 AM, Martin Liška wrote: > > Anyway that's not a candidate for DCE. I'm testing following patch. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > One alternative approach can be to drop DECL_SET_IS_OPERATOR_DELETE in: > cat -n gcc/cp/decl.c | less > ... > 4410 deltype = cp_build_type_attribute_variant (deltype, extvisattr); > 4411 deltype = build_exception_variant (deltype, empty_except_spec); > 4412 opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW); > 4413 DECL_SET_IS_OPERATOR_DELETE (opdel, true); > 4414 opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW); > 4415 DECL_SET_IS_OPERATOR_DELETE (opdel, true); > 4416 > 4417 if (flag_sized_deallocation) > 4418 { > 4419 /* operator delete (void *, size_t, align_val_t); */ > 4420 deltype = build_function_type_list (void_type_node, ptr_type_node, > 4421 size_type_node, align_type_node, > 4422 NULL_TREE); > 4423 deltype = cp_build_type_attribute_variant (deltype, extvisattr); > 4424 deltype = build_exception_variant (deltype, empty_except_spec); > 4425 opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW); > 4426 DECL_SET_IS_OPERATOR_DELETE (opdel, true); > 4427 opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW); > 4428 DECL_SET_IS_OPERATOR_DELETE (opdel, true); > 4429 } > 4430 } > > at lines 4426 and 4428. > > Richi what do you prefer? I don't understand why a "not simple" delete operator isn't fine to be DCEd? Does C++ somehow allow mismatching size specifications here? And what's the semantics then? Thus I'd rather go with your earlier patch to mark the op necessary. Richard. > Martin
On 7/30/19 10:40 AM, Richard Biener wrote: > On Tue, Jul 30, 2019 at 10:07 AM Martin Liška <mliska@suse.cz> wrote: >> >> On 7/30/19 9:46 AM, Martin Liška wrote: >>> Anyway that's not a candidate for DCE. I'm testing following patch. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> One alternative approach can be to drop DECL_SET_IS_OPERATOR_DELETE in: >> cat -n gcc/cp/decl.c | less >> ... >> 4410 deltype = cp_build_type_attribute_variant (deltype, extvisattr); >> 4411 deltype = build_exception_variant (deltype, empty_except_spec); >> 4412 opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW); >> 4413 DECL_SET_IS_OPERATOR_DELETE (opdel, true); >> 4414 opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW); >> 4415 DECL_SET_IS_OPERATOR_DELETE (opdel, true); >> 4416 >> 4417 if (flag_sized_deallocation) >> 4418 { >> 4419 /* operator delete (void *, size_t, align_val_t); */ >> 4420 deltype = build_function_type_list (void_type_node, ptr_type_node, >> 4421 size_type_node, align_type_node, >> 4422 NULL_TREE); >> 4423 deltype = cp_build_type_attribute_variant (deltype, extvisattr); >> 4424 deltype = build_exception_variant (deltype, empty_except_spec); >> 4425 opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW); >> 4426 DECL_SET_IS_OPERATOR_DELETE (opdel, true); >> 4427 opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW); >> 4428 DECL_SET_IS_OPERATOR_DELETE (opdel, true); >> 4429 } >> 4430 } >> >> at lines 4426 and 4428. >> >> Richi what do you prefer? > > I don't understand why a "not simple" delete operator isn't fine to be > DCEd? Does C++ > somehow allow mismatching size specifications here? No, they are the same. > And what's the semantics > then? > > Thus I'd rather go with your earlier patch to mark the op necessary. Ok, I'm sending tested patch. Ready for trunk? Thanks, Martin > > Richard. > >> Martin
On Tue, Jul 30, 2019 at 12:11 PM Martin Liška <mliska@suse.cz> wrote: > > On 7/30/19 10:40 AM, Richard Biener wrote: > > On Tue, Jul 30, 2019 at 10:07 AM Martin Liška <mliska@suse.cz> wrote: > >> > >> On 7/30/19 9:46 AM, Martin Liška wrote: > >>> Anyway that's not a candidate for DCE. I'm testing following patch. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> One alternative approach can be to drop DECL_SET_IS_OPERATOR_DELETE in: > >> cat -n gcc/cp/decl.c | less > >> ... > >> 4410 deltype = cp_build_type_attribute_variant (deltype, extvisattr); > >> 4411 deltype = build_exception_variant (deltype, empty_except_spec); > >> 4412 opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW); > >> 4413 DECL_SET_IS_OPERATOR_DELETE (opdel, true); > >> 4414 opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW); > >> 4415 DECL_SET_IS_OPERATOR_DELETE (opdel, true); > >> 4416 > >> 4417 if (flag_sized_deallocation) > >> 4418 { > >> 4419 /* operator delete (void *, size_t, align_val_t); */ > >> 4420 deltype = build_function_type_list (void_type_node, ptr_type_node, > >> 4421 size_type_node, align_type_node, > >> 4422 NULL_TREE); > >> 4423 deltype = cp_build_type_attribute_variant (deltype, extvisattr); > >> 4424 deltype = build_exception_variant (deltype, empty_except_spec); > >> 4425 opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW); > >> 4426 DECL_SET_IS_OPERATOR_DELETE (opdel, true); > >> 4427 opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW); > >> 4428 DECL_SET_IS_OPERATOR_DELETE (opdel, true); > >> 4429 } > >> 4430 } > >> > >> at lines 4426 and 4428. > >> > >> Richi what do you prefer? > > > > I don't understand why a "not simple" delete operator isn't fine to be > > DCEd? Does C++ > > somehow allow mismatching size specifications here? > > No, they are the same. > > > And what's the semantics > > then? > > > > Thus I'd rather go with your earlier patch to mark the op necessary. > > Ok, I'm sending tested patch. > > Ready for trunk? OK with the tests in if (gimple_call_builtin_p (stmt, BUILT_IN_FREE) - || (is_gimple_call (stmt) - && gimple_call_operator_delete_p (as_a <gcall *> (stmt)))) - + || is_delete_operator) exchanged (you already compuited is_delete_operator, no need to check for BUILT_IN_FREE if it is true). Richard. > Thanks, > Martin > > > > > Richard. > > > >> Martin >
+ /* Some delete operators have size as 2nd argument. */
Some delete operators have 3 arguments. From cp/decl.c:
/* operator delete (void *, size_t, align_val_t); */
On 7/30/19 1:35 PM, Marc Glisse wrote: > + /* Some delete operators have size as 2nd argument. */ > > Some delete operators have 3 arguments. From cp/decl.c: > > /* operator delete (void *, size_t, align_val_t); */ > Yep, I know. The patch I installed expects at least 2 arguments: + /* Some delete operators have size as 2nd argument. */ + if (is_delete_operator && gimple_call_num_args (stmt) >= 2) Martin
On Tue, 30 Jul 2019, Martin Liška wrote: > On 7/30/19 1:35 PM, Marc Glisse wrote: >> + /* Some delete operators have size as 2nd argument. */ >> >> Some delete operators have 3 arguments. From cp/decl.c: >> >> /* operator delete (void *, size_t, align_val_t); */ >> > > Yep, I know. The patch I installed expects at least 2 arguments: > > + /* Some delete operators have size as 2nd argument. */ > + if (is_delete_operator && gimple_call_num_args (stmt) >= 2) True, I guess I am a bit confused why the second argument (which could be either size or alignment) needs special handling (mark_operand_necessary) while the third one does not (it is usually a constant). I tried to experiment to understand, but it is complicated because including <new> disables the optimization: #include <new> void fn1() { char*p=new char; delete p; } This ICEs with -O -std=c++17: int a = 64; std::align_val_t b{64}; void fn1() { void *s = operator new(a,b); operator delete(s,8+*(unsigned long*)s,b); }
On 7/30/19 3:09 PM, Marc Glisse wrote: > On Tue, 30 Jul 2019, Martin Liška wrote: > >> On 7/30/19 1:35 PM, Marc Glisse wrote: >>> + /* Some delete operators have size as 2nd argument. */ >>> >>> Some delete operators have 3 arguments. From cp/decl.c: >>> >>> /* operator delete (void *, size_t, align_val_t); */ >>> >> >> Yep, I know. The patch I installed expects at least 2 arguments: >> >> + /* Some delete operators have size as 2nd argument. */ >> + if (is_delete_operator && gimple_call_num_args (stmt) >= 2) > > True, I guess I am a bit confused why the second argument (which could be either size or alignment) needs special handling (mark_operand_necessary) while the third one does not (it is usually a constant). Ah, that's bad, both of them need a care: diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c index bec13cd5930..80d5f5c30f7 100644 --- a/gcc/tree-ssa-dce.c +++ b/gcc/tree-ssa-dce.c @@ -824,13 +824,16 @@ propagate_necessity (bool aggressive) || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC)) || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee))) { - /* Some delete operators have size as 2nd argument. */ + /* Delete operators can have alignment and (or) size as next + arguments. When being a SSA_NAME, they must be marked + as necessary. */ if (is_delete_operator && gimple_call_num_args (stmt) >= 2) - { - tree size_argument = gimple_call_arg (stmt, 1); - if (TREE_CODE (size_argument) == SSA_NAME) - mark_operand_necessary (size_argument); - } + for (unsigned i = 1; i < gimple_call_num_args (stmt); i++) + { + tree arg = gimple_call_arg (stmt, i); + if (TREE_CODE (arg) == SSA_NAME) + mark_operand_necessary (arg); + } continue; } > > I tried to experiment to understand, but it is complicated because including <new> disables the optimization: > > #include <new> > void fn1() { > char*p=new char; > delete p; > } > > This ICEs with -O -std=c++17: > > int a = 64; > std::align_val_t b{64}; > void fn1() { > void *s = operator new(a,b); > operator delete(s,8+*(unsigned long*)s,b); > } > > I can't see it on current master. Can you? Martin
On Tue, 30 Jul 2019, Martin Liška wrote: > Ah, that's bad, both of them need a care: Yes, that makes more sense to me, thanks. >> I tried to experiment to understand, but it is complicated because including <new> disables the optimization: >> >> #include <new> >> void fn1() { >> char*p=new char; >> delete p; >> } >> >> This ICEs with -O -std=c++17: >> >> int a = 64; >> std::align_val_t b{64}; >> void fn1() { >> void *s = operator new(a,b); >> operator delete(s,8+*(unsigned long*)s,b); >> } >> >> > > I can't see it on current master. Can you? Yes. I just did a clean build to make sure.
On Tue, Jul 30, 2019 at 3:39 PM Martin Liška <mliska@suse.cz> wrote: > > On 7/30/19 3:09 PM, Marc Glisse wrote: > > On Tue, 30 Jul 2019, Martin Liška wrote: > > > >> On 7/30/19 1:35 PM, Marc Glisse wrote: > >>> + /* Some delete operators have size as 2nd argument. */ > >>> > >>> Some delete operators have 3 arguments. From cp/decl.c: > >>> > >>> /* operator delete (void *, size_t, align_val_t); */ > >>> > >> > >> Yep, I know. The patch I installed expects at least 2 arguments: > >> > >> + /* Some delete operators have size as 2nd argument. */ > >> + if (is_delete_operator && gimple_call_num_args (stmt) >= 2) > > > > True, I guess I am a bit confused why the second argument (which could be either size or alignment) needs special handling (mark_operand_necessary) while the third one does not (it is usually a constant). > > Ah, that's bad, both of them need a care: > > diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c > index bec13cd5930..80d5f5c30f7 100644 > --- a/gcc/tree-ssa-dce.c > +++ b/gcc/tree-ssa-dce.c > @@ -824,13 +824,16 @@ propagate_necessity (bool aggressive) > || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC)) > || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee))) > { > - /* Some delete operators have size as 2nd argument. */ > + /* Delete operators can have alignment and (or) size as next > + arguments. When being a SSA_NAME, they must be marked > + as necessary. */ > if (is_delete_operator && gimple_call_num_args (stmt) >= 2) > - { > - tree size_argument = gimple_call_arg (stmt, 1); > - if (TREE_CODE (size_argument) == SSA_NAME) > - mark_operand_necessary (size_argument); > - } > + for (unsigned i = 1; i < gimple_call_num_args (stmt); i++) > + { > + tree arg = gimple_call_arg (stmt, i); > + if (TREE_CODE (arg) == SSA_NAME) > + mark_operand_necessary (arg); > + } > > continue; > } Pre-approved. > > > > I tried to experiment to understand, but it is complicated because including <new> disables the optimization: > > > > #include <new> > > void fn1() { > > char*p=new char; > > delete p; > > } > > > > This ICEs with -O -std=c++17: > > > > int a = 64; > > std::align_val_t b{64}; > > void fn1() { > > void *s = operator new(a,b); > > operator delete(s,8+*(unsigned long*)s,b); > > } > > > > > > I can't see it on current master. Can you? > > Martin >
From 3d69c779ad5de447cd5ddba2595d2b1586dc5d3c Mon Sep 17 00:00:00 2001 From: Martin Liska <mliska@suse.cz> Date: Sun, 28 Jul 2019 13:04:28 +0200 Subject: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270). gcc/ChangeLog: 2019-07-28 Martin Liska <mliska@suse.cz> PR tree-optimization/91270 * tree-ssa-dce.c (eliminate_unnecessary_stmts): Delete also 2nd argument of a delete operator. gcc/testsuite/ChangeLog: 2019-07-28 Martin Liska <mliska@suse.cz> PR tree-optimization/91270 * g++.dg/torture/pr91270.C: New test. --- gcc/testsuite/g++.dg/torture/pr91270.C | 10 ++++++++++ gcc/tree-ssa-dce.c | 14 ++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 gcc/testsuite/g++.dg/torture/pr91270.C diff --git a/gcc/testsuite/g++.dg/torture/pr91270.C b/gcc/testsuite/g++.dg/torture/pr91270.C new file mode 100644 index 00000000000..60d766e9e9f --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr91270.C @@ -0,0 +1,10 @@ +/* { dg-do compile } */ + +struct S { + ~S(); +}; +int a = 123; +void fn1() { + S *s = new S[a]; + delete[] s; +} diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c index cf507fa0453..e5a1a9b7aa3 100644 --- a/gcc/tree-ssa-dce.c +++ b/gcc/tree-ssa-dce.c @@ -1294,6 +1294,20 @@ eliminate_unnecessary_stmts (void) && !gimple_plf (def_stmt, STMT_NECESSARY)) gimple_set_plf (stmt, STMT_NECESSARY, false); } + + /* Some delete operators have 2 arguments, where + the second argument is size of the deallocated memory. */ + if (gimple_call_num_args (stmt) == 2) + { + tree ptr = gimple_call_arg (stmt, 1); + if (TREE_CODE (ptr) == SSA_NAME) + { + 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); + } + } } /* If GSI is not necessary then remove it. */ -- 2.22.0