From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by sourceware.org (Postfix) with ESMTPS id 16B2C3858D33 for ; Tue, 5 Dec 2023 08:18:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 16B2C3858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 16B2C3858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.223.130 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701764333; cv=none; b=K3YppIHAGGcpd9/ZVK6xR03ezSrJSxVM3pY/x3/Vxi2mm6pQrV3uW5Hd3WLDLoCtmXY3Y8kZZ3VeCDwzuNKl0oUvZqiqiCBSbS5QEHjpzJluuxuBE8+J/2ggBxjJeA8onogQt/T2LU0idqZAnzw/zbgDf3lQU7jV2UYAkvDPqf0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701764333; c=relaxed/simple; bh=ZAryw3UTyF/dDAcvF/9e/7Pg7PgoD8jIrBbOcrV46h4=; h=DKIM-Signature:DKIM-Signature:Date:From:To:Subject:Message-ID: MIME-Version; b=Kr3xXq53JZjKJMjVQlZ2+YoBX3l3ILvhhqOpqa6mbNZI5urS4zxAOM5dODMWIe+5WwEyshFeF2WAdm+evDa8AlOtP99eTbIpw59ztD/zVYrKl/97TxRs9P7hkd/xImNqqpgDG6J+GZqGyDy1BVeVtGyxeEoqdMJwF2p7bzrjCok= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [10.168.4.150] (unknown [10.168.4.150]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id C855821CA3; Tue, 5 Dec 2023 08:18:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1701764330; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=DUd80UB0Eo42aDA2gPRjgKhkwK0CUDIH924JF0Dhlso=; b=liLtY/tIzwrAYtEQzOvZG/65E6jmAUhITWgQ8PMVDOZFU9Cmlm2WZRRedBR6DBRuZOzN3E Y7IlEJ/5NC52m78uAu8bsqggJul5VY8hH4r5s97HntJIvzXfJuEjbnHIlsseXlG3NUc+Vu mk85gZyeZ6Il9Uhx+NK2LknsMaDWGR0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1701764330; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=DUd80UB0Eo42aDA2gPRjgKhkwK0CUDIH924JF0Dhlso=; b=zyiNHlbe+KWNoCu7nKqPcZizpoQ8IEODX+BE4FEDZ5Jsv2Ozw5sMP0F+V8Bu5ZeXPQmh1Y /SUk9WwWfXq3xwDg== Date: Tue, 5 Dec 2023 09:15:06 +0100 (CET) From: Richard Biener To: Jakub Jelinek cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] lower-bitint: Make temporarily wrong IL less wrong [PR112843] In-Reply-To: Message-ID: <7sss7p84-8no9-3n67-on1r-790nor0pr781@fhfr.qr> References: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Authentication-Results: smtp-out1.suse.de; none X-Spam-Level: X-Spamd-Result: default: False [-4.30 / 50.00]; ARC_NA(0.00)[]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; MIME_GOOD(-0.10)[text/plain]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; NEURAL_HAM_SHORT(-0.20)[-1.000]; RCPT_COUNT_TWO(0.00)[2]; DBL_BLOCKED_OPENRESOLVER(0.00)[suse.de:email,gimple-lower-bitint.cc:url]; FUZZY_BLOCKED(0.00)[rspamd.com]; RCVD_COUNT_ZERO(0.00)[0]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; BAYES_HAM(-3.00)[100.00%] X-Spam-Score: -4.30 X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE,WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 > > 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 SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)