* [PATCH] lower-bitint: Make temporarily wrong IL less wrong [PR112843]
@ 2023-12-05 7:21 Jakub Jelinek
2023-12-05 8:05 ` [PATCH] lower-bitint, v2: " Jakub Jelinek
2023-12-05 8:15 ` [PATCH] lower-bitint: " Richard Biener
0 siblings, 2 replies; 3+ messages in thread
From: Jakub Jelinek @ 2023-12-05 7:21 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
Hi!
As discussed in the PR, for the middle (on x86-64 65..128 bit) _BitInt
types like
_1 = x_4(D) * 5;
where _1 and x_4(D) have _BitInt(128) type and x is PARM_DECL, the bitint
lowering pass wants to replace this with
_13 = (int128_t) x_4(D);
_12 = _13 * 5;
_1 = (_BitInt(128)) _12;
where _13 and _12 have int128_t type and the ranger ICEs when the IL is
temporarily invalid:
during GIMPLE pass: bitintlower
pr112843.c: In function ‘foo’:
pr112843.c:7:1: internal compiler error: Segmentation fault
7 | foo (_BitInt (128) x, _BitInt (256) y)
| ^~~
0x152943f crash_signal
../../gcc/toplev.cc:316
0x25c21c8 ranger_cache::range_of_expr(vrange&, tree_node*, gimple*)
../../gcc/gimple-range-cache.cc:1204
0x25cdcf9 fold_using_range::range_of_range_op(vrange&, gimple_range_op_handler&, fur_source&)
../../gcc/gimple-range-fold.cc:671
0x25cf9a0 fold_using_range::fold_stmt(vrange&, gimple*, fur_source&, tree_node*)
../../gcc/gimple-range-fold.cc:602
0x25b5520 gimple_ranger::update_stmt(gimple*)
../../gcc/gimple-range.cc:564
0x16f1234 update_stmt_operands(function*, gimple*)
../../gcc/tree-ssa-operands.cc:1150
0x117a5b6 update_stmt_if_modified(gimple*)
../../gcc/gimple-ssa.h:187
0x117a5b6 update_stmt_if_modified(gimple*)
../../gcc/gimple-ssa.h:184
0x117a5b6 update_modified_stmt
../../gcc/gimple-iterator.cc:44
0x117a5b6 gsi_insert_after(gimple_stmt_iterator*, gimple*, gsi_iterator_update)
../../gcc/gimple-iterator.cc:544
0x25abc2f gimple_lower_bitint
../../gcc/gimple-lower-bitint.cc:6348
What the code does right now is, it first creates a new SSA_NAME (_12
above), adds the
_1 = (_BitInt(128)) _12;
stmt after it (where it crashes, because _12 has no SSA_NAME_DEF_STMT yet),
then sets lhs of the previous stmt to _12 (this is also temporarily
incorrect, there are incompatible types involved in the stmt), later on
changes also operands and finally update_stmt it.
The following patch instead changes the lhs of the stmt before adding the
cast after it. The question is if this is less or more wrong temporarily
(but the ICE is gone).
Yet another possibility would be to first adjust the operands of stmt
(without update_stmt), then set_lhs to a new lhs (still without
update_stmt), then add the cast after it and finally update_stmt (stmt).
Maybe that would be less wrong (still, before it is updated some chains
might think it is still the setter of _1 when it is not anymore).
Anyway, should I go with that order then instead of the patch below?
The reason I tweaked the lhs first is that it then just uses gimple_op and
iterates over all ops, if that is done before lhs it would need to special
case which op to skip because it is lhs (I'm using gimple_get_lhs for the
lhs, but this isn't done for GIMPLE_CALL nor GIMPLE_PHI, so GIMPLE_ASSIGN
or say GIMPLE_GOTO etc. are the only options, so I could just start with
op 1 rather than 0 for is_gimple_assign).
2023-12-04 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/112843
* gimple-lower-bitint.cc (gimple_lower_bitint): Change lhs of stmt
to lhs2 before building and inserting lhs = (cast) lhs2; assignment.
* gcc.dg/bitint-47.c: New test.
--- gcc/gimple-lower-bitint.cc.jj 2023-12-03 17:53:55.604855552 +0100
+++ gcc/gimple-lower-bitint.cc 2023-12-04 14:39:20.352057389 +0100
@@ -6338,6 +6338,7 @@ gimple_lower_bitint (void)
int uns = TYPE_UNSIGNED (TREE_TYPE (lhs));
type = build_nonstandard_integer_type (prec, uns);
tree lhs2 = make_ssa_name (type);
+ gimple_set_lhs (stmt, lhs2);
gimple *g = gimple_build_assign (lhs, NOP_EXPR, lhs2);
if (stmt_ends_bb_p (stmt))
{
@@ -6346,7 +6347,6 @@ gimple_lower_bitint (void)
}
else
gsi_insert_after (&gsi, g, GSI_SAME_STMT);
- gimple_set_lhs (stmt, lhs2);
}
unsigned int nops = gimple_num_ops (stmt);
for (unsigned int i = 0; i < nops; ++i)
--- gcc/testsuite/gcc.dg/bitint-47.c.jj 2023-12-04 14:53:19.784200724 +0100
+++ gcc/testsuite/gcc.dg/bitint-47.c 2023-12-04 14:42:07.251699994 +0100
@@ -0,0 +1,13 @@
+/* PR tree-optimization/112843 */
+/* { dg-do compile { target bitint } } */
+/* { dg-options "-O2" } */
+
+#if __BITINT_MAXWIDTH__ >= 256
+_BitInt (256)
+foo (_BitInt (128) x, _BitInt (256) y)
+{
+ return x * 5 * y;
+}
+#else
+int x;
+#endif
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] lower-bitint, v2: Make temporarily wrong IL less wrong [PR112843]
2023-12-05 7:21 [PATCH] lower-bitint: Make temporarily wrong IL less wrong [PR112843] Jakub Jelinek
@ 2023-12-05 8:05 ` Jakub Jelinek
2023-12-05 8:15 ` [PATCH] lower-bitint: " Richard Biener
1 sibling, 0 replies; 3+ messages in thread
From: Jakub Jelinek @ 2023-12-05 8:05 UTC (permalink / raw)
To: Richard Biener, gcc-patches
On Tue, Dec 05, 2023 at 08:21:15AM +0100, Jakub Jelinek wrote:
> The reason I tweaked the lhs first is that it then just uses gimple_op and
> iterates over all ops, if that is done before lhs it would need to special
> case which op to skip because it is lhs (I'm using gimple_get_lhs for the
> lhs, but this isn't done for GIMPLE_CALL nor GIMPLE_PHI, so GIMPLE_ASSIGN
> or say GIMPLE_GOTO etc. are the only options, so I could just start with
> op 1 rather than 0 for is_gimple_assign).
Here is a variant which adjusts the inputs first before set_lhs, then
addition of cast after it and finally update_stmt.
So far tested with
make check-gcc check-g++ -j32 -k GCC_TEST_RUN_EXPENSIVE=1 RUNTESTFLAGS="GCC_TEST_RUN_EXPENSIVE=1 dg.exp='*bitint* pr112673.c builtin-stdc-bit-*.c pr112566-2.c pr112511.c' dg-torture.exp=*bitint* dfp.exp=*bitint*"
2023-12-05 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/112843
* gimple-lower-bitint.cc (gimple_lower_bitint): Change lhs of stmt
to lhs2 before building and inserting lhs = (cast) lhs2; assignment.
Adjust stmt operands before adjusting lhs.
* gcc.dg/bitint-47.c: New test.
--- gcc/gimple-lower-bitint.cc.jj 2023-12-03 17:53:55.604855552 +0100
+++ gcc/gimple-lower-bitint.cc 2023-12-05 08:53:13.424763429 +0100
@@ -6329,27 +6329,9 @@ gimple_lower_bitint (void)
tree type = NULL_TREE;
/* Middle _BitInt(N) is rewritten to casts to INTEGER_TYPEs
with the same precision and back. */
- if (tree lhs = gimple_get_lhs (stmt))
- if (TREE_CODE (TREE_TYPE (lhs)) == BITINT_TYPE
- && (bitint_precision_kind (TREE_TYPE (lhs))
- == bitint_prec_middle))
- {
- int prec = TYPE_PRECISION (TREE_TYPE (lhs));
- int uns = TYPE_UNSIGNED (TREE_TYPE (lhs));
- type = build_nonstandard_integer_type (prec, uns);
- tree lhs2 = make_ssa_name (type);
- gimple *g = gimple_build_assign (lhs, NOP_EXPR, lhs2);
- if (stmt_ends_bb_p (stmt))
- {
- edge e = find_fallthru_edge (gsi_bb (gsi)->succs);
- gsi_insert_on_edge_immediate (e, g);
- }
- else
- gsi_insert_after (&gsi, g, GSI_SAME_STMT);
- gimple_set_lhs (stmt, lhs2);
- }
unsigned int nops = gimple_num_ops (stmt);
- for (unsigned int i = 0; i < nops; ++i)
+ for (unsigned int i = is_gimple_assign (stmt) ? 1 : 0;
+ i < nops; ++i)
if (tree op = gimple_op (stmt, i))
{
tree nop = maybe_cast_middle_bitint (&gsi, op, type);
@@ -6376,6 +6358,25 @@ gimple_lower_bitint (void)
type);
}
}
+ if (tree lhs = gimple_get_lhs (stmt))
+ if (TREE_CODE (TREE_TYPE (lhs)) == BITINT_TYPE
+ && (bitint_precision_kind (TREE_TYPE (lhs))
+ == bitint_prec_middle))
+ {
+ int prec = TYPE_PRECISION (TREE_TYPE (lhs));
+ int uns = TYPE_UNSIGNED (TREE_TYPE (lhs));
+ type = build_nonstandard_integer_type (prec, uns);
+ tree lhs2 = make_ssa_name (type);
+ gimple_set_lhs (stmt, lhs2);
+ gimple *g = gimple_build_assign (lhs, NOP_EXPR, lhs2);
+ if (stmt_ends_bb_p (stmt))
+ {
+ edge e = find_fallthru_edge (gsi_bb (gsi)->succs);
+ gsi_insert_on_edge_immediate (e, g);
+ }
+ else
+ gsi_insert_after (&gsi, g, GSI_SAME_STMT);
+ }
update_stmt (stmt);
continue;
}
--- gcc/testsuite/gcc.dg/bitint-47.c.jj 2023-12-04 14:53:19.784200724 +0100
+++ gcc/testsuite/gcc.dg/bitint-47.c 2023-12-04 14:42:07.251699994 +0100
@@ -0,0 +1,13 @@
+/* PR tree-optimization/112843 */
+/* { dg-do compile { target bitint } } */
+/* { dg-options "-O2" } */
+
+#if __BITINT_MAXWIDTH__ >= 256
+_BitInt (256)
+foo (_BitInt (128) x, _BitInt (256) y)
+{
+ return x * 5 * y;
+}
+#else
+int x;
+#endif
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] lower-bitint: Make temporarily wrong IL less wrong [PR112843]
2023-12-05 7:21 [PATCH] lower-bitint: Make temporarily wrong IL less wrong [PR112843] Jakub Jelinek
2023-12-05 8:05 ` [PATCH] lower-bitint, v2: " Jakub Jelinek
@ 2023-12-05 8:15 ` Richard Biener
1 sibling, 0 replies; 3+ messages in thread
From: Richard Biener @ 2023-12-05 8:15 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Tue, 5 Dec 2023, Jakub Jelinek wrote:
> Hi!
>
> As discussed in the PR, for the middle (on x86-64 65..128 bit) _BitInt
> types like
> _1 = x_4(D) * 5;
> where _1 and x_4(D) have _BitInt(128) type and x is PARM_DECL, the bitint
> lowering pass wants to replace this with
> _13 = (int128_t) x_4(D);
> _12 = _13 * 5;
> _1 = (_BitInt(128)) _12;
> where _13 and _12 have int128_t type and the ranger ICEs when the IL is
> temporarily invalid:
> during GIMPLE pass: bitintlower
> pr112843.c: In function ?foo?:
> pr112843.c:7:1: internal compiler error: Segmentation fault
> 7 | foo (_BitInt (128) x, _BitInt (256) y)
> | ^~~
> 0x152943f crash_signal
> ../../gcc/toplev.cc:316
> 0x25c21c8 ranger_cache::range_of_expr(vrange&, tree_node*, gimple*)
> ../../gcc/gimple-range-cache.cc:1204
> 0x25cdcf9 fold_using_range::range_of_range_op(vrange&, gimple_range_op_handler&, fur_source&)
> ../../gcc/gimple-range-fold.cc:671
> 0x25cf9a0 fold_using_range::fold_stmt(vrange&, gimple*, fur_source&, tree_node*)
> ../../gcc/gimple-range-fold.cc:602
> 0x25b5520 gimple_ranger::update_stmt(gimple*)
> ../../gcc/gimple-range.cc:564
> 0x16f1234 update_stmt_operands(function*, gimple*)
> ../../gcc/tree-ssa-operands.cc:1150
> 0x117a5b6 update_stmt_if_modified(gimple*)
> ../../gcc/gimple-ssa.h:187
> 0x117a5b6 update_stmt_if_modified(gimple*)
> ../../gcc/gimple-ssa.h:184
> 0x117a5b6 update_modified_stmt
> ../../gcc/gimple-iterator.cc:44
> 0x117a5b6 gsi_insert_after(gimple_stmt_iterator*, gimple*, gsi_iterator_update)
> ../../gcc/gimple-iterator.cc:544
> 0x25abc2f gimple_lower_bitint
> ../../gcc/gimple-lower-bitint.cc:6348
>
> What the code does right now is, it first creates a new SSA_NAME (_12
> above), adds the
> _1 = (_BitInt(128)) _12;
> stmt after it (where it crashes, because _12 has no SSA_NAME_DEF_STMT yet),
> then sets lhs of the previous stmt to _12 (this is also temporarily
> incorrect, there are incompatible types involved in the stmt), later on
> changes also operands and finally update_stmt it.
>
> The following patch instead changes the lhs of the stmt before adding the
> cast after it. The question is if this is less or more wrong temporarily
> (but the ICE is gone).
> Yet another possibility would be to first adjust the operands of stmt
> (without update_stmt), then set_lhs to a new lhs (still without
> update_stmt), then add the cast after it and finally update_stmt (stmt).
> Maybe that would be less wrong (still, before it is updated some chains
> might think it is still the setter of _1 when it is not anymore).
> Anyway, should I go with that order then instead of the patch below?
There isn't really anything "wrong" with the code. All stmt modification
is stmt-local, so is (or rather should be ...) update_stmt.
So the question is more one of readability of the code performing the
update, not how intermediate states of GIMPLE look like.
I have meanwhile successfully tested removal of that
range_query->update_stmt call in update_stmt_operands and will push it
after writing a changelog entry.
As for the patch whatever you think improves readability should be OK.
Richard.
> The reason I tweaked the lhs first is that it then just uses gimple_op and
> iterates over all ops, if that is done before lhs it would need to special
> case which op to skip because it is lhs (I'm using gimple_get_lhs for the
> lhs, but this isn't done for GIMPLE_CALL nor GIMPLE_PHI, so GIMPLE_ASSIGN
> or say GIMPLE_GOTO etc. are the only options, so I could just start with
> op 1 rather than 0 for is_gimple_assign).
>
> 2023-12-04 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/112843
> * gimple-lower-bitint.cc (gimple_lower_bitint): Change lhs of stmt
> to lhs2 before building and inserting lhs = (cast) lhs2; assignment.
>
> * gcc.dg/bitint-47.c: New test.
>
> --- gcc/gimple-lower-bitint.cc.jj 2023-12-03 17:53:55.604855552 +0100
> +++ gcc/gimple-lower-bitint.cc 2023-12-04 14:39:20.352057389 +0100
> @@ -6338,6 +6338,7 @@ gimple_lower_bitint (void)
> int uns = TYPE_UNSIGNED (TREE_TYPE (lhs));
> type = build_nonstandard_integer_type (prec, uns);
> tree lhs2 = make_ssa_name (type);
> + gimple_set_lhs (stmt, lhs2);
> gimple *g = gimple_build_assign (lhs, NOP_EXPR, lhs2);
> if (stmt_ends_bb_p (stmt))
> {
> @@ -6346,7 +6347,6 @@ gimple_lower_bitint (void)
> }
> else
> gsi_insert_after (&gsi, g, GSI_SAME_STMT);
> - gimple_set_lhs (stmt, lhs2);
> }
> unsigned int nops = gimple_num_ops (stmt);
> for (unsigned int i = 0; i < nops; ++i)
> --- gcc/testsuite/gcc.dg/bitint-47.c.jj 2023-12-04 14:53:19.784200724 +0100
> +++ gcc/testsuite/gcc.dg/bitint-47.c 2023-12-04 14:42:07.251699994 +0100
> @@ -0,0 +1,13 @@
> +/* PR tree-optimization/112843 */
> +/* { dg-do compile { target bitint } } */
> +/* { dg-options "-O2" } */
> +
> +#if __BITINT_MAXWIDTH__ >= 256
> +_BitInt (256)
> +foo (_BitInt (128) x, _BitInt (256) y)
> +{
> + return x * 5 * y;
> +}
> +#else
> +int x;
> +#endif
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-12-05 8:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-05 7:21 [PATCH] lower-bitint: Make temporarily wrong IL less wrong [PR112843] Jakub Jelinek
2023-12-05 8:05 ` [PATCH] lower-bitint, v2: " Jakub Jelinek
2023-12-05 8:15 ` [PATCH] lower-bitint: " Richard Biener
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).