Re: [PATCH] c++: devirtualization of array destruction [PR110057]
On 7/26/23 20:06, Ng YongXiang wrote:
Hi Jason,
I've made the following changes.
1. Add pr83054-2.C
2. Move the devirt tests to tree-ssa.
3. Remove dg do run for devirt tests
4. Add // PR c++/110057
5. Generate commit message with git gcc-commit-mklog
6. Check commit format with git gcc-verify
Thanks!
Thanks. I added a comment and fixed another test that was breaking with
the patch; here's what I pushed.
JasonFrom a47e615fbf9c6f4b24e5032df5d720b6bf9b63b5 Mon Sep 17 00:00:00 2001
From: Ng YongXiang
Date: Thu, 27 Jul 2023 08:06:14 +0800
Subject: [PATCH] c++: devirtualization of array destruction [PR110057]
To: gcc-patches@gcc.gnu.org
PR c++/110057
PR ipa/83054
gcc/cp/ChangeLog:
* init.cc (build_vec_delete_1): Devirtualize array destruction.
gcc/testsuite/ChangeLog:
* g++.dg/warn/pr83054.C: Remove devirtualization warning.
* g++.dg/lto/pr89335_0.C: Likewise.
* g++.dg/tree-ssa/devirt-array-destructor-1.C: New test.
* g++.dg/tree-ssa/devirt-array-destructor-2.C: New test.
* g++.dg/warn/pr83054-2.C: New test.
Signed-off-by: Ng Yong Xiang
---
gcc/cp/init.cc| 11 +++--
gcc/testsuite/g++.dg/lto/pr89335_0.C | 2 +-
.../tree-ssa/devirt-array-destructor-1.C | 28
.../tree-ssa/devirt-array-destructor-2.C | 29
gcc/testsuite/g++.dg/warn/pr83054-2.C | 44 +++
gcc/testsuite/g++.dg/warn/pr83054.C | 2 +-
6 files changed, 110 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-1.C
create mode 100644 gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-2.C
create mode 100644 gcc/testsuite/g++.dg/warn/pr83054-2.C
diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
index ff5014ca576..3b9a7783391 100644
--- a/gcc/cp/init.cc
+++ b/gcc/cp/init.cc
@@ -4116,8 +4116,8 @@ build_vec_delete_1 (location_t loc, tree base, tree maxindex, tree type,
if (type_build_dtor_call (type))
{
tmp = build_delete (loc, ptype, base, sfk_complete_destructor,
- LOOKUP_NORMAL|LOOKUP_DESTRUCTOR, 1,
- complain);
+ LOOKUP_NORMAL|LOOKUP_DESTRUCTOR|LOOKUP_NONVIRTUAL,
+ 1, complain);
if (tmp == error_mark_node)
return error_mark_node;
}
@@ -4146,9 +4146,12 @@ build_vec_delete_1 (location_t loc, tree base, tree maxindex, tree type,
if (tmp == error_mark_node)
return error_mark_node;
body = build_compound_expr (loc, body, tmp);
+ /* [expr.delete]/3: "In an array delete expression, if the dynamic type of
+ the object to be deleted is not similar to its static type, the behavior
+ is undefined." So we can set LOOKUP_NONVIRTUAL. */
tmp = build_delete (loc, ptype, tbase, sfk_complete_destructor,
- LOOKUP_NORMAL|LOOKUP_DESTRUCTOR, 1,
- complain);
+ LOOKUP_NORMAL|LOOKUP_DESTRUCTOR|LOOKUP_NONVIRTUAL,
+ 1, complain);
if (tmp == error_mark_node)
return error_mark_node;
body = build_compound_expr (loc, body, tmp);
diff --git a/gcc/testsuite/g++.dg/lto/pr89335_0.C b/gcc/testsuite/g++.dg/lto/pr89335_0.C
index 95bf4b3b0cb..76382f8d742 100644
--- a/gcc/testsuite/g++.dg/lto/pr89335_0.C
+++ b/gcc/testsuite/g++.dg/lto/pr89335_0.C
@@ -9,7 +9,7 @@ public:
virtual ~Container ();
};
-class List : public Container // { dg-lto-message "final would enable devirtualization" }
+class List : public Container
{
};
diff --git a/gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-1.C b/gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-1.C
new file mode 100644
index 000..ce8dc2a57cd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-1.C
@@ -0,0 +1,28 @@
+// PR c++/110057
+/* { dg-do-compile } */
+/* Virtual calls should be devirtualized because we know dynamic type of object in array at compile time */
+/* { dg-options "-O3 -fdump-tree-optimized -fno-inline" } */
+
+class A
+{
+public:
+ virtual ~A()
+ {
+ }
+};
+
+class B : public A
+{
+public:
+ virtual ~B()
+ {
+ }
+};
+
+int main()
+{
+ B b[10];
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "optimized"} } */
diff --git a/gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-2.C b/gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-2.C
new file mode 100644
index 000..6b44dc1a4ee
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-2.C
@@ -0,0 +1,29 @@
+// PR c++/110057
+/* { dg-do-compile } */
+/* Virtual calls should be devirtualized because we know dynamic type of object in array at compile time */
+/* { dg-options "-O3 -fdump-tree-optimized -fno-inline" } */
+
+class A
+{
+public:
+ virtual ~A()
+ {
+ }
+};
+
+class B : public A
+{
+public:
+ virtual ~B()
+ {
+ }
+};
+
+int main()
+{
+ B* ptr = new B[10];
+ delete[] ptr;
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "optimized"} } */
diff --git a/gcc/te
Re: [PATCH] c++: devirtualization of array destruction [PR110057]
Hi Jason,
I've made the following changes.
1. Add pr83054-2.C
2. Move the devirt tests to tree-ssa.
3. Remove dg do run for devirt tests
4. Add // PR c++/110057
5. Generate commit message with git gcc-commit-mklog
6. Check commit format with git gcc-verify
Thanks!
On Thu, Jul 27, 2023 at 12:30 AM Jason Merrill wrote:
> On 7/26/23 12:00, Ng YongXiang wrote:
> > Hi Jason,
> >
> > Thanks for the reply and review. I've attached an updated patch with the
> > change log and sign off.
> >
> > The change made in gcc/testsuite/g++.dg/warn/pr83054.C is because I
> > think there is no more warning since we have already devirtualized the
> > destruction for the array.
>
> Makes sense, and it's good to have your adjusted testcase in the
> testsuite, it should just be a new one (maybe pr83054-2.C).
>
> > Apologies for the poor formatting. It is my first time contributing. Do
> > let me know if there's any stuff I've missed and feel free to modify the
> > patch where you deem necessary.
>
> No worries!
>
> The ChangeLog entries still need some adjustment, according to git
> gcc-verify (from contrib/gcc-git-customization.sh, see
> https://gcc.gnu.org/gitwrite.html):
>
> ERR: line should start with a tab: "* init.c: Call non virtual
> destructor of objects in array"
> ERR: line should start with a tab: "*
> g++.dg/devirt-array-destructor-1.C: New."
> ERR: line should start with a tab: "*
> g++.dg/devirt-array-destructor-2.C: New."
> ERR: line should start with a tab: "* g++.dg/warn/pr83054.C:
> Remove expected warnings caused by devirtualization"
> ERR: PR 110057 in subject but not in changelog: "c++: devirtualization
> of array destruction [PR110057]"
>
> git gcc-commit-mklog (also from gcc-git-customization.sh) makes
> generating ChangeLog entries a lot simpler.
>
> > * g++.dg/devirt-array-destructor-1.C: New.
>
> Tests that look at tree-optimization dump files should go in the
> g++.dg/tree-ssa subdirectory.
>
> > +/* { dg-do run } */
>
> It seems unnecessary to execute these tests, I'd think the default of {
> dg-do compile } would be fine.
>
> It's also good to have a
>
> // PR c++/110057
>
> line at the top of the testcase for future reference. gcc-commit-mklog
> also uses that to add the PR number to the ChangeLog.
>
> Jason
>
>
From 04e9b412e49a3966f84edd3afda66ebdb729efdc Mon Sep 17 00:00:00 2001
From: yongxiangng
Date: Thu, 27 Jul 2023 08:01:42 +0800
Subject: [PATCH 1/1] [PATCH] c++: devirtualization of array destruction
[PR110057]
PR c++/110057
PR ipa/83054
gcc/cp/ChangeLog:
* init.cc (build_vec_delete_1): Devirtualize array destruction.
gcc/testsuite/ChangeLog:
* g++.dg/warn/pr83054.C: Remove devirtualization warning.
* g++.dg/tree-ssa/devirt-array-destructor-1.C: New test.
* g++.dg/tree-ssa/devirt-array-destructor-2.C: New test.
* g++.dg/warn/pr83054-2.C: New test.
Signed-off-by: Ng Yong Xiang
---
gcc/cp/init.cc| 8 ++--
.../tree-ssa/devirt-array-destructor-1.C | 28
.../tree-ssa/devirt-array-destructor-2.C | 29
gcc/testsuite/g++.dg/warn/pr83054-2.C | 44 +++
gcc/testsuite/g++.dg/warn/pr83054.C | 2 +-
5 files changed, 106 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-1.C
create mode 100644 gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-2.C
create mode 100644 gcc/testsuite/g++.dg/warn/pr83054-2.C
diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
index 6ccda365b04..69ab51d0a4b 100644
--- a/gcc/cp/init.cc
+++ b/gcc/cp/init.cc
@@ -4112,8 +4112,8 @@ build_vec_delete_1 (location_t loc, tree base, tree maxindex, tree type,
if (type_build_dtor_call (type))
{
tmp = build_delete (loc, ptype, base, sfk_complete_destructor,
- LOOKUP_NORMAL|LOOKUP_DESTRUCTOR, 1,
- complain);
+ LOOKUP_NORMAL|LOOKUP_DESTRUCTOR|LOOKUP_NONVIRTUAL,
+ 1, complain);
if (tmp == error_mark_node)
return error_mark_node;
}
@@ -4143,8 +4143,8 @@ build_vec_delete_1 (location_t loc, tree base, tree maxindex, tree type,
return error_mark_node;
body = build_compound_expr (loc, body, tmp);
tmp = build_delete (loc, ptype, tbase, sfk_complete_destructor,
- LOOKUP_NORMAL|LOOKUP_DESTRUCTOR, 1,
- complain);
+ LOOKUP_NORMAL|LOOKUP_DESTRUCTOR|LOOKUP_NONVIRTUAL,
+ 1, complain);
if (tmp == error_mark_node)
return error_mark_node;
body = build_compound_expr (loc, body, tmp);
diff --git a/gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-1.C b/gcc/testsuite/g++.dg/tree-ssa/devirt-array-destructor-1.C
new file mode 100644
index 000..ce8dc2a57cd
--- /dev/null
+
Re: [PATCH] c++: devirtualization of array destruction [PR110057]
On 7/26/23 12:00, Ng YongXiang wrote:
Hi Jason,
Thanks for the reply and review. I've attached an updated patch with the
change log and sign off.
The change made in gcc/testsuite/g++.dg/warn/pr83054.C is because I
think there is no more warning since we have already devirtualized the
destruction for the array.
Makes sense, and it's good to have your adjusted testcase in the
testsuite, it should just be a new one (maybe pr83054-2.C).
Apologies for the poor formatting. It is my first time contributing. Do
let me know if there's any stuff I've missed and feel free to modify the
patch where you deem necessary.
No worries!
The ChangeLog entries still need some adjustment, according to git
gcc-verify (from contrib/gcc-git-customization.sh, see
https://gcc.gnu.org/gitwrite.html):
ERR: line should start with a tab: "* init.c: Call non virtual
destructor of objects in array"
ERR: line should start with a tab: "*
g++.dg/devirt-array-destructor-1.C: New."
ERR: line should start with a tab: "*
g++.dg/devirt-array-destructor-2.C: New."
ERR: line should start with a tab: "* g++.dg/warn/pr83054.C:
Remove expected warnings caused by devirtualization"
ERR: PR 110057 in subject but not in changelog: "c++: devirtualization
of array destruction [PR110057]"
git gcc-commit-mklog (also from gcc-git-customization.sh) makes
generating ChangeLog entries a lot simpler.
* g++.dg/devirt-array-destructor-1.C: New.
Tests that look at tree-optimization dump files should go in the
g++.dg/tree-ssa subdirectory.
+/* { dg-do run } */
It seems unnecessary to execute these tests, I'd think the default of {
dg-do compile } would be fine.
It's also good to have a
// PR c++/110057
line at the top of the testcase for future reference. gcc-commit-mklog
also uses that to add the PR number to the ChangeLog.
Jason
[PATCH] c++: devirtualization of array destruction [PR110057]
Hi Jason,
Thanks for the reply and review. I've attached an updated patch with the
change log and sign off.
The change made in gcc/testsuite/g++.dg/warn/pr83054.C is because I think
there is no more warning since we have already devirtualized the
destruction for the array.
Apologies for the poor formatting. It is my first time contributing. Do let
me know if there's any stuff I've missed and feel free to modify the patch
where you deem necessary.
Thanks!
On Wed, Jul 26, 2023 at 12:25 PM Jason Merrill wrote:
> On 7/12/23 10:10, Ng YongXiang via Gcc-patches wrote:
> > Component:
> > c++
> >
> > Bug ID:
> > 110057
> >
> > Bugzilla link:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110057
> >
> > Description:
> > Array should not call virtual destructor of object when array is
> destructed
> >
> > ChangeLog:
> >
> > 2023-07-12 Ng YongXiang PR c++
> > * Devirtualize auto generated destructor calls of arraycp/*
> > init.c: Call non virtual destructor of objects in arraytestsuite/
> >* g++.dg/devirt-array-destructor-1.C: New.*
> > g++.dg/devirt-array-destructor-2.C: New.
> >
> >
> > On Wed, Jul 12, 2023 at 5:02 PM Xi Ruoyao wrote:
> >
> >> On Wed, 2023-07-12 at 16:58 +0800, Ng YongXiang via Gcc-patches wrote:
> >>> I'm writing to seek for a review for an issue I filed some time ago.
> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110057 . A proposed patch
> >> is
> >>> attached in the bug tracker as well.
> >>
> >> You should send the patch to gcc-patches@gcc.gnu.org for a review, see
> >> https://gcc.gnu.org/contribute.html for the details. Generally we
> >> consider patches attached in bugzilla as drafts.
>
> Thanks! The change makes sense under
> https://eel.is/c++draft/expr.delete#3.sentence-2 , but please look again
> at contribute.html.
>
> In particular, the Legal section; you don't seem to have a copyright
> assignment with the FSF, nor do I see a DCO certification
> (https://gcc.gnu.org/dco.html) in your patch.
>
> Like the examples in contribute.html, the subject line should be more
> like "[PATCH] c++: devirtualization of array destruction [PR110057]"
>
> The ChangeLog entry should be in the commit message.
>
> > * g++.dg/warn/pr83054.C: Change expected number of
> devirtualized calls
>
> This isn't just changing the expected number, it's also changing the
> array from a local variable to dynamically allocated, which is a big
> change to what's being tested. If you want to test the dynamic case,
> please add a new test instead of making this change.
>
> > diff --git a/gcc/testsuite/g++.dg/warn/pr83054.C
> b/gcc/testsuite/g++.dg/warn/pr83054.C
> > index 5285f94acee..7cd0951713d 100644
> > --- a/gcc/testsuite/g++.dg/warn/pr83054.C
> > +++ b/gcc/testsuite/g++.dg/warn/pr83054.C
> > @@ -10,7 +10,7 @@
> > #endif
> >
> > extern "C" int printf (const char *, ...);
> > -struct foo // { dg-warning "final would enable devirtualization of 5
> calls" }
> > +struct foo // { dg-warning "final would enable devirtualization of 1
> call" }
> > {
> >static int count;
> >void print (int i, int j) { printf ("foo[%d][%d] = %d\n", i, j, x); }
> > @@ -29,19 +29,15 @@ int foo::count;
> >
> > int main ()
> > {
> > - {
> > -foo array[3][3];
> > -for (int i = 0; i < 3; i++)
> > - {
> > - for (int j = 0; j < 3; j++)
> > - {
> > - printf("[%d][%d] = %x\n", i, j, (void *)[i][j]);
> > - }
> > - }
> > - // The count should be nine, if not, fail the test.
> > - if (foo::count != 9)
> > - return 1;
> > - }
> > + foo* arr[9];
> > + for (int i = 0; i < 9; ++i)
> > +arr[i] = new foo();
> > + if (foo::count != 9)
> > +return 1;
> > + for (int i = 0; i < 9; ++i)
> > +arr[i]->print(i / 3, i % 3);
> > + for (int i = 0; i < 9; ++i)
> > +delete arr[i];
>
>
>
From 2b7cbd8e0787c8e20f0464dbf610908a9f3c68f7 Mon Sep 17 00:00:00 2001
From: yongxiangng
Date: Wed, 26 Jul 2023 23:45:25 +0800
Subject: [PATCH 1/1] [PATCH] c++: devirtualization of array destruction
[PR110057]
cp/ChangeLog:
* init.c: Call non virtual destructor of objects in array
testsuite/ChangeLog:
* g++.dg/devirt-array-destructor-1.C: New.
* g++.dg/devirt-array-destructor-2.C: New.
* g++.dg/warn/pr83054.C: Remove expected warnings caused by devirtualization
Signed-off-by
