Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
On Tue, Jul 30, 2019 at 3:39 PM Martin Liška 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 disables the optimization:
> >
> > #include
> > 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
>
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
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
disables the optimization:
#include
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.
--
Marc Glisse
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
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
> disables the optimization:
>
> #include
> 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
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
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 disables the optimization:
#include
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);
}
--
Marc Glisse
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
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
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
+ /* 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); */ -- Marc Glisse
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
On Tue, Jul 30, 2019 at 12:11 PM Martin Liška wrote:
>
> On 7/30/19 10:40 AM, Richard Biener wrote:
> > On Tue, Jul 30, 2019 at 10:07 AM Martin Liška 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 (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
>
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
On 7/30/19 10:40 AM, Richard Biener wrote:
> On Tue, Jul 30, 2019 at 10:07 AM Martin Liška 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
>From b4645189743b2670f77028933086fff56c46eb75 Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Sun, 28 Jul 2019 13:04:28 +0200
Subject: [PATCH] Mark 2nd argument of delete operator as needed (PR
tree-optimization/91270).
gcc/cp/ChangeLog:
2019-07-30 Martin Liska
PR tree-optimization/91270
* tree-ssa-dce.c (propagate_necessity): Mark 2nd argument
of delete operator as needed.
gcc/testsuite/ChangeLog:
2019-07-30 Martin Liska
PR tree-optimization/91270
* g++.dg/torture/pr91270.C: New test.
---
gcc/testsuite/g++.dg/torture/pr91270.C | 10 ++
gcc/tree-ssa-dce.c | 19 +++
2 files changed, 25 insertions(+), 4 deletions(-)
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 000..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 763b76f0e53..c816fccceb4 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -804,10 +804,11 @@ propagate_necessity (bool aggressive)
/* If this is a call to free which is directly fed by an
allocation function do not mark that necessary through
processing the argument. */
+ bool is_delete_operator
+ = (is_gimple_call (stmt)
+ && gimple_call_operator_delete_p (as_a (stmt)));
if (gimple_call_builtin_p (stmt, BUILT_IN_FREE)
- || (is_gimple_call (stmt)
- && gimple_call_operator_delete_p (as_a (stmt
-
+ || is_delete_operator)
{
tree ptr = gimple_call_arg (stmt, 0);
gimple *def_stmt;
@@ -822,7 +823,17 @@ 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 size as 2nd argument. */
+ 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);
+ }
+
+ continue;
+ }
}
FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
--
2.22.0
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
On Tue, Jul 30, 2019 at 10:07 AM Martin Liška 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
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
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
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
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 ; else goto ;
:
iftmp.0 = D.2341 + 8;
goto ;
:
iftmp.0 = 18446744073709551615;
:
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
{
:
if (D.2326 < 0) goto ; else goto ;
:
S::S (D.2325);
D.2325 = D.2325 + 1;
D.2326 = D.2326 + -1;
goto ;
:
}
catch
{
{
struct S * D.2336;
if (D.2324 != 0B) goto ; else goto ;
:
_3 = (sizetype) D.2326;
_4 = D.2341 - _3;
_5 = _4 + 18446744073709551615;
D.2336 = D.2324 + _5;
:
if (D.2336 == D.2324) goto ; else goto ;
:
D.2336 = D.2336 + 18446744073709551615;
S::~S (D.2336);
goto ;
:
goto ;
:
:
}
}
retval.2 = D.2324;
}
catch
{
if (D.2341 <= 9223372036854775800) goto ; else goto ;
:
iftmp.3 = D.2341 + 8;
goto ;
:
iftmp.3 = 18446744073709551615;
:
operator delete [] (D.2323, iftmp.3);
}
s = D.2323 + 8;
{
struct S * D.2337;
if (s != 0B) goto ; else goto ;
:
try
{
_6 = s + 18446744073709551608;
_7 = *_6;
D.2337 = s + _7;
:
if (D.2337 == s) goto ; else goto ;
:
D.2337 = D.2337 + 18446744073709551615;
S::~S (D.2337);
goto ;
:
}
finally
{
_8 = s + 18446744073709551608;
_9 = *_8;
_10 = _9 + 8;
_11 = s + 18446744073709551608;
operator delete [] (_11, _10);
}
goto ;
:
:
}
}
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
>From 312626229bfd4162550891bd8947b0fe310da6f5 Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Tue, 30 Jul 2019 09:24:56 +0200
Subject: [PATCH] DCE: do not handle delete operators with a SSA_NAME as a size
argument (PR tree-optimization/91270).
gcc/ChangeLog:
2019-07-30 Martin Liska
PR tree-optimization/91270
* tree-ssa-dce.c (simple_delete_operator_p): New.
(propagate_necessity): Use it to filter delete operators
that we want to delete.
(eliminate_unnecessary_stmts): Likewise.
gcc/testsuite/ChangeLog:
2019-07-30 Martin Liska
PR tree-optimization/91270
* g++.dg/torture/pr91270.C: New test.
---
gcc/testsuite/g++.dg/torture/pr91270.C | 10 ++
gcc/tree-ssa-dce.c | 19 ++-
2 files changed, 24 insertions(+), 5 deletions(-)
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 000..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 763b76f0e53..e844824dc12 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -646,6 +646,18 @@ degenerate_phi_p (gimple *phi)
return true;
}
+/* Return true when a GIMPLE STMT is a delete call operator that
+ has either one argument or second argument is an integer constant. */
+
+static bool
+simple_delete_operator_p (gimple *stmt)
+{
+ return (is_gimple_call (stmt)
+ && (gimple_call_operator_delete_p (as_a (stmt))
+ && (gimple_call_num_args (stmt) == 1
+ || TREE_CODE (gimple_call_arg (stmt, 1)) == INTEGER_CST)));
+}
+
/* Propagate necessity using the operands of necessary statements.
Process the uses on each statement in the worklist, and add all
feeding statements which contribute to the calculation of this
@@ -805,9 +817,7 @@ propagate_necessity (bool aggressive)
allocation function do not mark that necessary through
processing the argument. */
if (gimple_call_builtin_p (stmt, BUILT_IN_FREE)
- || (is_gimple_call (stmt)
- && gimple_call_operator_delete_p (as_a
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
On Mon, Jul 29, 2019 at 12:37 PM Martin Liška wrote:
>
> On 7/29/19 11:59 AM, Richard Biener wrote:
> > On Sun, Jul 28, 2019 at 4:57 PM Martin Liška 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:
>
>:
> # iftmp.0_15 = PHI
> _23 = operator new [] (iftmp.0_15);
> _24 = _23;
> MEM[(sizetype *)_24] = _19;
> _26 = _24 + 8;
> _27 = _26;
> _2 = _19 + 18446744073709551615;
> _28 = (long int) _2;
>
>:
> # _12 = PHI <_27(5), _29(7)>
> # _13 = PHI <_28(5), _30(7)>
> if (_13 < 0)
> goto ; [INV]
> else
> goto ; [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
>
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
On 7/29/19 11:59 AM, Richard Biener wrote:
> On Sun, Jul 28, 2019 at 4:57 PM Martin Liška 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:
:
# iftmp.0_15 = PHI
_23 = operator new [] (iftmp.0_15);
_24 = _23;
MEM[(sizetype *)_24] = _19;
_26 = _24 + 8;
_27 = _26;
_2 = _19 + 18446744073709551615;
_28 = (long int) _2;
:
# _12 = PHI <_27(5), _29(7)>
# _13 = PHI <_28(5), _30(7)>
if (_13 < 0)
goto ; [INV]
else
goto ; [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
Re: [PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
On Sun, Jul 28, 2019 at 4:57 PM Martin Liška 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
[PATCH] Remove also 2nd argument for unused delete operator (PR tree-optimization/91270).
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?
Thanks,
Martin
>From 3d69c779ad5de447cd5ddba2595d2b1586dc5d3c Mon Sep 17 00:00:00 2001
From: Martin Liska
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
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
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 000..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
