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 <ja...@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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
