* [PATCH] c++: Fix ANNOTATE_EXPR instantiation [PR114409]
@ 2024-04-10 13:06 Jakub Jelinek
2024-04-10 16:39 ` Jason Merrill
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2024-04-10 13:06 UTC (permalink / raw)
To: Jason Merrill, Patrick Palka; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3842 bytes --]
Hi!
The following testcase ICEs starting with the r14-4229 PR111529
change which moved ANNOTATE_EXPR handling from tsubst_expr to
tsubst_copy_and_build.
ANNOTATE_EXPR is only allowed in the IL to wrap a loop condition,
and the loop condition of while/for loops can be a COMPOUND_EXPR
with DECL_EXPR in the first operand and the corresponding VAR_DECL
in the second, as created by finish_cond
else if (!empty_expr_stmt_p (cond))
expr = build2 (COMPOUND_EXPR, TREE_TYPE (expr), cond, expr);
Since then Patrick reworked the instantiation, so that we have now
tsubst_stmt and tsubst_expr and ANNOTATE_EXPR ended up in the latter,
while only tsubst_stmt can handle DECL_EXPR.
Now, the reason why the while/for loops with variable declaration
in the condition works in templates without the pragmas (i.e. without
ANNOTATE_EXPR) is that both the FOR_STMT and WHILE_STMT handling uses
RECUR aka tsubst_stmt in handling of the *_COND operand:
case FOR_STMT:
stmt = begin_for_stmt (NULL_TREE, NULL_TREE);
RECUR (FOR_INIT_STMT (t));
finish_init_stmt (stmt);
tmp = RECUR (FOR_COND (t));
finish_for_cond (tmp, stmt, false, 0, false);
and
case WHILE_STMT:
stmt = begin_while_stmt ();
tmp = RECUR (WHILE_COND (t));
finish_while_stmt_cond (tmp, stmt, false, 0, false);
Therefore, it will handle DECL_EXPR embedded in COMPOUND_EXPR of the
{WHILE,FOR}_COND just fine.
But if that COMPOUND_EXPR with DECL_EXPR is wrapped with one or more
ANNOTATE_EXPRs, because ANNOTATE_EXPR is now done solely in tsubst_expr
and uses RECUR there (i.e. tsubst_expr), it will ICE on DECL_EXPR in there.
Here are 2 possible fixes for this.
The first one keeps ANNOTATE_EXPR handling in tsubst_expr but uses
tsubst_stmt for the first operand.
The second one moves ANNOTATE_EXPR handling to tsubst_stmt (and uses
tsubst_expr for the second/third operand (it could just RECUR too if you
prefer that)).
Yet another possibility could be to duplicate the ANNOTATE_EXPR handling
from tsubst_expr to tsubst_stmt, where both would just RECUR on its
operands, so if one arrives to ANNOTATE_EXPR from tsubst_stmt, it will
tsubst_stmt recursively, if from tsubst_expr (when?) then it would handle
it using tsubst_expr.
So far just lightly tested (but g++.dg/ext/unroll-4.C and the new test
both pass with both versions of the patch), what do you prefer? I'd like
to avoid testing too many variants...
2024-04-10 Jakub Jelinek <jakub@redhat.com>
PR c++/114409
* pt.cc (tsubst_expr) <case ANNOTATE_EXPR>: Use tsubst_stmt rather
than tsubst_expr aka RECUR on op1.
* g++.dg/ext/pr114409-2.C: New test.
--- gcc/cp/pt.cc.jj 2024-04-09 09:29:04.721521726 +0200
+++ gcc/cp/pt.cc 2024-04-10 14:38:43.591554947 +0200
@@ -21774,7 +21774,10 @@ tsubst_expr (tree t, tree args, tsubst_f
case ANNOTATE_EXPR:
{
- op1 = RECUR (TREE_OPERAND (t, 0));
+ /* ANNOTATE_EXPR should only appear in WHILE_COND, DO_COND or
+ FOR_COND expressions, which are tsubsted using tsubst_stmt
+ rather than tsubst_expr and can contain DECL_EXPRs. */
+ op1 = tsubst_stmt (TREE_OPERAND (t, 0), args, complain, in_decl);
tree op2 = RECUR (TREE_OPERAND (t, 1));
tree op3 = RECUR (TREE_OPERAND (t, 2));
if (TREE_CODE (op2) == INTEGER_CST
--- gcc/testsuite/g++.dg/ext/pr114409-2.C.jj 2024-04-10 14:35:19.693300552 +0200
+++ gcc/testsuite/g++.dg/ext/pr114409-2.C 2024-04-10 14:35:13.513383766 +0200
@@ -0,0 +1,36 @@
+// PR c++/114409
+// { dg-do compile }
+// { dg-options "-O2" }
+
+template <typename T>
+T
+foo (T)
+{
+ static T t;
+ return 42 - ++t;
+}
+
+template <typename T>
+void
+bar (T x)
+{
+ #pragma GCC novector
+ while (T y = foo (x))
+ ++y;
+}
+
+template <typename T>
+void
+baz (T x)
+{
+ #pragma GCC novector
+ for (; T y = foo (x); )
+ ++y;
+}
+
+void
+qux ()
+{
+ bar (0);
+ baz (0);
+}
Jakub
[-- Attachment #2: Q414b --]
[-- Type: text/plain, Size: 2583 bytes --]
2024-04-10 Jakub Jelinek <jakub@redhat.com>
PR c++/114409
* pt.cc (tsubst_expr) <case ANNOTATE_EXPR>: Move to ...
(tsubst_stmt) <case ANNOTATE_EXPR>: ... here. Use tsubst_expr
instead of RECUR for the last 2 arguments.
* g++.dg/ext/pr114409-2.C: New test.
--- gcc/cp/pt.cc.jj 2024-04-09 09:29:04.721521726 +0200
+++ gcc/cp/pt.cc 2024-04-10 14:45:25.527142692 +0200
@@ -19433,6 +19433,23 @@ tsubst_stmt (tree t, tree args, tsubst_f
case PREDICT_EXPR:
RETURN (add_stmt (copy_node (t)));
+ case ANNOTATE_EXPR:
+ {
+ /* Although ANNOTATE_EXPR is an expression, it can only appear in
+ WHILE_COND, DO_COND or FOR_COND expressions, which are tsubsted
+ using tsubst_stmt rather than tsubst_expr and can contain
+ DECL_EXPRs. */
+ tree op1 = RECUR (TREE_OPERAND (t, 0));
+ tree op2 = tsubst_expr (TREE_OPERAND (t, 1), args, complain, in_decl);
+ tree op3 = tsubst_expr (TREE_OPERAND (t, 2), args, complain, in_decl);
+ if (TREE_CODE (op2) == INTEGER_CST
+ && wi::to_widest (op2) == (int) annot_expr_unroll_kind)
+ op3 = cp_check_pragma_unroll (EXPR_LOCATION (TREE_OPERAND (t, 2)),
+ op3);
+ RETURN (build3_loc (EXPR_LOCATION (t), ANNOTATE_EXPR,
+ TREE_TYPE (op1), op1, op2, op3));
+ }
+
default:
gcc_assert (!STATEMENT_CODE_P (TREE_CODE (t)));
@@ -21772,19 +21789,6 @@ tsubst_expr (tree t, tree args, tsubst_f
RETURN (op);
}
- case ANNOTATE_EXPR:
- {
- op1 = RECUR (TREE_OPERAND (t, 0));
- tree op2 = RECUR (TREE_OPERAND (t, 1));
- tree op3 = RECUR (TREE_OPERAND (t, 2));
- if (TREE_CODE (op2) == INTEGER_CST
- && wi::to_widest (op2) == (int) annot_expr_unroll_kind)
- op3 = cp_check_pragma_unroll (EXPR_LOCATION (TREE_OPERAND (t, 2)),
- op3);
- RETURN (build3_loc (EXPR_LOCATION (t), ANNOTATE_EXPR,
- TREE_TYPE (op1), op1, op2, op3));
- }
-
default:
/* Handle Objective-C++ constructs, if appropriate. */
if (tree subst = objcp_tsubst_expr (t, args, complain, in_decl))
--- gcc/testsuite/g++.dg/ext/pr114409-2.C.jj 2024-04-10 14:35:19.693300552 +0200
+++ gcc/testsuite/g++.dg/ext/pr114409-2.C 2024-04-10 14:35:13.513383766 +0200
@@ -0,0 +1,36 @@
+// PR c++/114409
+// { dg-do compile }
+// { dg-options "-O2" }
+
+template <typename T>
+T
+foo (T)
+{
+ static T t;
+ return 42 - ++t;
+}
+
+template <typename T>
+void
+bar (T x)
+{
+ #pragma GCC novector
+ while (T y = foo (x))
+ ++y;
+}
+
+template <typename T>
+void
+baz (T x)
+{
+ #pragma GCC novector
+ for (; T y = foo (x); )
+ ++y;
+}
+
+void
+qux ()
+{
+ bar (0);
+ baz (0);
+}
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] c++: Fix ANNOTATE_EXPR instantiation [PR114409]
2024-04-10 13:06 [PATCH] c++: Fix ANNOTATE_EXPR instantiation [PR114409] Jakub Jelinek
@ 2024-04-10 16:39 ` Jason Merrill
0 siblings, 0 replies; 2+ messages in thread
From: Jason Merrill @ 2024-04-10 16:39 UTC (permalink / raw)
To: Jakub Jelinek, Patrick Palka; +Cc: gcc-patches
On 4/10/24 09:06, Jakub Jelinek wrote:
> Hi!
>
> The following testcase ICEs starting with the r14-4229 PR111529
> change which moved ANNOTATE_EXPR handling from tsubst_expr to
> tsubst_copy_and_build.
> ANNOTATE_EXPR is only allowed in the IL to wrap a loop condition,
> and the loop condition of while/for loops can be a COMPOUND_EXPR
> with DECL_EXPR in the first operand and the corresponding VAR_DECL
> in the second, as created by finish_cond
> else if (!empty_expr_stmt_p (cond))
> expr = build2 (COMPOUND_EXPR, TREE_TYPE (expr), cond, expr);
> Since then Patrick reworked the instantiation, so that we have now
> tsubst_stmt and tsubst_expr and ANNOTATE_EXPR ended up in the latter,
> while only tsubst_stmt can handle DECL_EXPR.
>
> Now, the reason why the while/for loops with variable declaration
> in the condition works in templates without the pragmas (i.e. without
> ANNOTATE_EXPR) is that both the FOR_STMT and WHILE_STMT handling uses
> RECUR aka tsubst_stmt in handling of the *_COND operand:
> case FOR_STMT:
> stmt = begin_for_stmt (NULL_TREE, NULL_TREE);
> RECUR (FOR_INIT_STMT (t));
> finish_init_stmt (stmt);
> tmp = RECUR (FOR_COND (t));
> finish_for_cond (tmp, stmt, false, 0, false);
> and
> case WHILE_STMT:
> stmt = begin_while_stmt ();
> tmp = RECUR (WHILE_COND (t));
> finish_while_stmt_cond (tmp, stmt, false, 0, false);
> Therefore, it will handle DECL_EXPR embedded in COMPOUND_EXPR of the
> {WHILE,FOR}_COND just fine.
> But if that COMPOUND_EXPR with DECL_EXPR is wrapped with one or more
> ANNOTATE_EXPRs, because ANNOTATE_EXPR is now done solely in tsubst_expr
> and uses RECUR there (i.e. tsubst_expr), it will ICE on DECL_EXPR in there.
>
> Here are 2 possible fixes for this.
> The first one keeps ANNOTATE_EXPR handling in tsubst_expr but uses
> tsubst_stmt for the first operand.
> The second one moves ANNOTATE_EXPR handling to tsubst_stmt (and uses
> tsubst_expr for the second/third operand (it could just RECUR too if you
> prefer that)).
> Yet another possibility could be to duplicate the ANNOTATE_EXPR handling
> from tsubst_expr to tsubst_stmt, where both would just RECUR on its
> operands, so if one arrives to ANNOTATE_EXPR from tsubst_stmt, it will
> tsubst_stmt recursively, if from tsubst_expr (when?) then it would handle
> it using tsubst_expr.
>
> So far just lightly tested (but g++.dg/ext/unroll-4.C and the new test
> both pass with both versions of the patch), what do you prefer? I'd like
> to avoid testing too many variants...
Let's go with the second.
> 2024-04-10 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/114409
> * pt.cc (tsubst_expr) <case ANNOTATE_EXPR>: Use tsubst_stmt rather
> than tsubst_expr aka RECUR on op1.
>
> * g++.dg/ext/pr114409-2.C: New test.
>
> --- gcc/cp/pt.cc.jj 2024-04-09 09:29:04.721521726 +0200
> +++ gcc/cp/pt.cc 2024-04-10 14:38:43.591554947 +0200
> @@ -21774,7 +21774,10 @@ tsubst_expr (tree t, tree args, tsubst_f
>
> case ANNOTATE_EXPR:
> {
> - op1 = RECUR (TREE_OPERAND (t, 0));
> + /* ANNOTATE_EXPR should only appear in WHILE_COND, DO_COND or
> + FOR_COND expressions, which are tsubsted using tsubst_stmt
> + rather than tsubst_expr and can contain DECL_EXPRs. */
> + op1 = tsubst_stmt (TREE_OPERAND (t, 0), args, complain, in_decl);
> tree op2 = RECUR (TREE_OPERAND (t, 1));
> tree op3 = RECUR (TREE_OPERAND (t, 2));
> if (TREE_CODE (op2) == INTEGER_CST
> --- gcc/testsuite/g++.dg/ext/pr114409-2.C.jj 2024-04-10 14:35:19.693300552 +0200
> +++ gcc/testsuite/g++.dg/ext/pr114409-2.C 2024-04-10 14:35:13.513383766 +0200
> @@ -0,0 +1,36 @@
> +// PR c++/114409
> +// { dg-do compile }
> +// { dg-options "-O2" }
> +
> +template <typename T>
> +T
> +foo (T)
> +{
> + static T t;
> + return 42 - ++t;
> +}
> +
> +template <typename T>
> +void
> +bar (T x)
> +{
> + #pragma GCC novector
> + while (T y = foo (x))
> + ++y;
> +}
> +
> +template <typename T>
> +void
> +baz (T x)
> +{
> + #pragma GCC novector
> + for (; T y = foo (x); )
> + ++y;
> +}
> +
> +void
> +qux ()
> +{
> + bar (0);
> + baz (0);
> +}
>
> Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-04-10 16:39 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-10 13:06 [PATCH] c++: Fix ANNOTATE_EXPR instantiation [PR114409] Jakub Jelinek
2024-04-10 16:39 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).