On 10/7/24 2:45 PM, Marek Polacek wrote:
On Wed, Oct 02, 2024 at 05:52:13PM -0400, Jason Merrill wrote:
On 10/2/24 3:20 PM, Marek Polacek wrote:
On Sat, Sep 28, 2024 at 08:39:12AM +0200, Jakub Jelinek wrote:
On Fri, Sep 27, 2024 at 04:01:33PM +0200, Jakub Jelinek wrote:
So, I think we should go with (but so far completely untested except
for pr78687.C which is optimized with Marek's patch and the above testcase
which doesn't have the clearing anymore) the following patch.
That patch had a bug in type_has_padding_at_level_p and so it didn't
bootstrap.
Here is a full patch which does.
[...]
And here's my patch, bootstrapped/regtested on x86_64-pc-linux-gnu
on top of Jakub's patch, ok for trunk once the prerequisite is in?
-- >8 --
This PR reports a missed optimization. When we have:
Str str{"Test"};
callback(str);
as in the test, we're able to evaluate the Str::Str() call at compile
time. But when we have:
callback(Str{"Test"});
we are not. With this patch (in fact, it's Patrick's patch with a little
tweak), we turn
callback (TARGET_EXPR <D.2890, <<< Unknown tree: aggr_init_expr
5
__ct_comp
D.2890
(struct Str *) <<< Unknown tree: void_cst >>>
(const char *) "Test" >>>>)
into
callback (TARGET_EXPR <D.2890, {.str=(const char *) "Test", .length=4}>)
I explored the idea of calling maybe_constant_value for the whole
TARGET_EXPR in cp_fold. That has three problems:
- we can't always elide a TARGET_EXPR, so we'd have to make sure the
result is also a TARGET_EXPR;
- the resulting TARGET_EXPR must have the same flags, otherwise Bad
Things happen;
- getting a new slot is also problematic. I've seen a test where we
had "TARGET_EXPR<D.2680, ...>, D.2680", and folding the whole TARGET_EXPR
would get us "TARGET_EXPR<D.2681, ...>", but since we don't see the outer
D.2680, we can't replace it with D.2681, and things break.
With this patch, two tree-ssa tests regressed: pr78687.C and pr90883.C.
FAIL: g++.dg/tree-ssa/pr90883.C scan-tree-dump dse1 "Deleted redundant store: .*.a
= {}"
is easy. Previously, we would call C::C, so .gimple has:
D.2590 = {};
C::C (&D.2590);
D.2597 = D.2590;
return D.2597;
Then .einline inlines the C::C call:
D.2590 = {};
D.2590.a = {}; // #1
D.2590.b = 0; // #2
D.2597 = D.2590;
D.2590 ={v} {CLOBBER(eos)};
return D.2597;
then #2 is removed in .fre1, and #1 is removed in .dse1. So the test
passes. But with the patch, .gimple won't have that C::C call, so the
IL is of course going to look different. The .optimized dump looks the
same though so there's no problem.
pr78687.C was fixed by Jakub's categorize_ctor_elements_1 patch.
PR c++/116416
gcc/cp/ChangeLog:
* cp-gimplify.cc (cp_fold_r) <case TARGET_EXPR>: Try to fold
TARGET_EXPR_INITIAL and replace it with the folded result if
it's TREE_CONSTANT.
gcc/testsuite/ChangeLog:
* g++.dg/analyzer/pr97116.C: Adjust dg-message.
* g++.dg/tree-ssa/pr90883.C: Adjust dg-final.
* g++.dg/cpp0x/constexpr-prvalue1.C: New test.
* g++.dg/cpp1y/constexpr-prvalue1.C: New test.
Co-authored-by: Patrick Palka <ppa...@redhat.com>
---
gcc/cp/cp-gimplify.cc | 10 +++++++
gcc/testsuite/g++.dg/analyzer/pr97116.C | 2 +-
.../g++.dg/cpp0x/constexpr-prvalue1.C | 24 +++++++++++++++
.../g++.dg/cpp1y/constexpr-prvalue1.C | 30 +++++++++++++++++++
gcc/testsuite/g++.dg/tree-ssa/pr90883.C | 4 +--
5 files changed, 67 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-prvalue1.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-prvalue1.C
diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
index 003e68f1ea7..c63fdf3edd1 100644
--- a/gcc/cp/cp-gimplify.cc
+++ b/gcc/cp/cp-gimplify.cc
@@ -1473,6 +1473,16 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void *data_)
that case, strip it in favor of this one. */
if (tree &init = TARGET_EXPR_INITIAL (stmt))
{
+ if ((data->flags & ff_genericize)
Why only with ff_genericize?
No reason AFAICT. Dropped.
+ && !flag_no_inline)
+ {
+ tree folded = maybe_constant_init (init, TARGET_EXPR_SLOT (stmt));
+ if (folded != init && TREE_CONSTANT (folded))
+ {
+ init = folded;
+ break;
Are you sure we never need the TARGET_EXPR_CLEANUP walk in this case?
No.
Maybe move the TARGET_EXPR_CLEANUP walk and the *walk_subtrees = 0 before
this new code? And the "folding might replace" comment down to the
tree_code == target_expr block?
Like this?
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk once the
prerequisite is in?
OK.
-- >8 --
This PR reports a missed optimization. When we have:
Str str{"Test"};
callback(str);
as in the test, we're able to evaluate the Str::Str() call at compile
time. But when we have:
callback(Str{"Test"});
we are not. With this patch (in fact, it's Patrick's patch with a little
tweak), we turn
callback (TARGET_EXPR <D.2890, <<< Unknown tree: aggr_init_expr
5
__ct_comp
D.2890
(struct Str *) <<< Unknown tree: void_cst >>>
(const char *) "Test" >>>>)
into
callback (TARGET_EXPR <D.2890, {.str=(const char *) "Test", .length=4}>)
I explored the idea of calling maybe_constant_value for the whole
TARGET_EXPR in cp_fold. That has three problems:
- we can't always elide a TARGET_EXPR, so we'd have to make sure the
result is also a TARGET_EXPR;
- the resulting TARGET_EXPR must have the same flags, otherwise Bad
Things happen;
- getting a new slot is also problematic. I've seen a test where we
had "TARGET_EXPR<D.2680, ...>, D.2680", and folding the whole TARGET_EXPR
would get us "TARGET_EXPR<D.2681, ...>", but since we don't see the outer
D.2680, we can't replace it with D.2681, and things break.
With this patch, two tree-ssa tests regressed: pr78687.C and pr90883.C.
FAIL: g++.dg/tree-ssa/pr90883.C scan-tree-dump dse1 "Deleted redundant store: .*.a
= {}"
is easy. Previously, we would call C::C, so .gimple has:
D.2590 = {};
C::C (&D.2590);
D.2597 = D.2590;
return D.2597;
Then .einline inlines the C::C call:
D.2590 = {};
D.2590.a = {}; // #1
D.2590.b = 0; // #2
D.2597 = D.2590;
D.2590 ={v} {CLOBBER(eos)};
return D.2597;
then #2 is removed in .fre1, and #1 is removed in .dse1. So the test
passes. But with the patch, .gimple won't have that C::C call, so the
IL is of course going to look different. The .optimized dump looks the
same though so there's no problem.
pr78687.C was fixed by Jakub's categorize_ctor_elements_1 patch.
PR c++/116416
gcc/cp/ChangeLog:
* cp-gimplify.cc (cp_fold_r) <case TARGET_EXPR>: Try to fold
TARGET_EXPR_INITIAL and replace it with the folded result if
it's TREE_CONSTANT.
gcc/testsuite/ChangeLog:
* g++.dg/analyzer/pr97116.C: Adjust dg-message.
* g++.dg/tree-ssa/pr90883.C: Adjust dg-final.
* g++.dg/cpp0x/constexpr-prvalue1.C: New test.
* g++.dg/cpp1y/constexpr-prvalue1.C: New test.
Co-authored-by: Patrick Palka <ppa...@redhat.com>
---
gcc/cp/cp-gimplify.cc | 10 +++++--
gcc/testsuite/g++.dg/analyzer/pr97116.C | 2 +-
.../g++.dg/cpp0x/constexpr-prvalue1.C | 24 +++++++++++++++
.../g++.dg/cpp1y/constexpr-prvalue1.C | 30 +++++++++++++++++++
gcc/testsuite/g++.dg/tree-ssa/pr90883.C | 4 +--
5 files changed, 65 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-prvalue1.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-prvalue1.C
diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
index 003e68f1ea7..d45f76c5685 100644
--- a/gcc/cp/cp-gimplify.cc
+++ b/gcc/cp/cp-gimplify.cc
@@ -1469,13 +1469,19 @@ cp_fold_r (tree *stmt_p, int *walk_subtrees, void
*data_)
if (data->flags & ff_genericize)
cp_genericize_target_expr (stmt_p);
- /* Folding might replace e.g. a COND_EXPR with a TARGET_EXPR; in
- that case, strip it in favor of this one. */
if (tree &init = TARGET_EXPR_INITIAL (stmt))
{
cp_walk_tree (&init, cp_fold_r, data, NULL);
cp_walk_tree (&TARGET_EXPR_CLEANUP (stmt), cp_fold_r, data, NULL);
*walk_subtrees = 0;
+ if (!flag_no_inline)
+ {
+ tree folded = maybe_constant_init (init, TARGET_EXPR_SLOT (stmt));
+ if (folded != init && TREE_CONSTANT (folded))
+ init = folded;
+ }
+ /* Folding might replace e.g. a COND_EXPR with a TARGET_EXPR; in
+ that case, strip it in favor of this one. */
if (TREE_CODE (init) == TARGET_EXPR)
{
tree sub = TARGET_EXPR_INITIAL (init);
diff --git a/gcc/testsuite/g++.dg/analyzer/pr97116.C
b/gcc/testsuite/g++.dg/analyzer/pr97116.C
index d8e08a73172..1c404c2ceb2 100644
--- a/gcc/testsuite/g++.dg/analyzer/pr97116.C
+++ b/gcc/testsuite/g++.dg/analyzer/pr97116.C
@@ -16,7 +16,7 @@ struct foo
void test_1 (void)
{
foo *p = new(NULL) foo (42); // { dg-warning "non-null expected" "warning" }
- // { dg-message "argument 'this' \\(\[^\n\]*\\) NULL where non-null expected"
"final event" { target *-*-* } .-1 }
+ // { dg-message "argument 'this'( \\(\[^\n\]*\\))? NULL where non-null expected"
"final event" { target *-*-* } .-1 }
}
int test_2 (void)
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-prvalue1.C
b/gcc/testsuite/g++.dg/cpp0x/constexpr-prvalue1.C
new file mode 100644
index 00000000000..f09088d41e8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-prvalue1.C
@@ -0,0 +1,24 @@
+// PR c++/116416
+// { dg-do compile { target c++11 } }
+// { dg-options "-O" }
+
+struct optional {
+ constexpr optional(int) {}
+};
+optional foo() { return 2; }
+
+
+struct C {
+ constexpr C(int) {}
+};
+
+struct B {
+ C fn(int) { return 0; }
+};
+
+void
+g ()
+{
+ B b;
+ b.fn(0);
+}
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-prvalue1.C
b/gcc/testsuite/g++.dg/cpp1y/constexpr-prvalue1.C
new file mode 100644
index 00000000000..ad31e300116
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-prvalue1.C
@@ -0,0 +1,30 @@
+// PR c++/116416
+// { dg-do compile { target c++14 } }
+// { dg-options "-O" }
+
+struct Str {
+ constexpr Str() {}
+ constexpr Str(const char *instr) {
+ str = instr; length = 0;
+ for (auto index = 0; instr[index]; ++index) {
+ ++length;
+ }
+ }
+ const char *str = nullptr;
+ int length = 0;
+};
+extern void callback(Str str);
+void
+func1()
+{
+ callback(Str{"Test"});
+}
+void
+func2()
+{
+ Str str{"Test"};
+ callback(str);
+}
+
+// Check that we don't call Str::Str(char const*)
+// { dg-final { scan-assembler-not "_ZN3StrC1EPKc" } }
diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr90883.C
b/gcc/testsuite/g++.dg/tree-ssa/pr90883.C
index 37df17d0b16..ad9231eaff2 100644
--- a/gcc/testsuite/g++.dg/tree-ssa/pr90883.C
+++ b/gcc/testsuite/g++.dg/tree-ssa/pr90883.C
@@ -16,5 +16,5 @@
// We want to match enough here to capture that we deleted an empty
// constructor store
// mips will expand to loop to clear because CLEAR_RATIO.
-// { dg-final { scan-tree-dump "Deleted redundant store: .*\.a = {}" "dse1" {
xfail { mips*-*-* } } } }
-
+// { dg-final { scan-tree-dump-not ".*\.a = {}" "dse1" { xfail { mips*-*-* } }
} }
+// { dg-final { scan-tree-dump-not ".*\.b = 0" "dse1" { xfail { mips*-*-* } }
} }
base-commit: 9a17e6d03c6ed53e3b2dfd2c3ff9b1066ffa97b9
