From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 6AA733858D35 for ; Wed, 21 Jun 2023 10:36:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6AA733858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 868E821B50; Wed, 21 Jun 2023 10:36:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1687343808; 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=PjZM1emXuyU0oBbL9WujytDw0AbsoHKhZ1aw0xB5jpc=; b=xM56Pcj/zEUeFjTuh4Fp3waw8NiSLmhemigl7S4/Nd23KuqncLFAJGK3+dphQsdid9O6wV q9GEhA7VwXUe3hqsRCxcRseaxBYVk3fBzLLB7X+z5Tj+y5AO6cA/zaGUW0sPLYULhwlgys W8EL2AMO7j/CwBHzYOCcxYE/yRj/mB4= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1687343808; 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=PjZM1emXuyU0oBbL9WujytDw0AbsoHKhZ1aw0xB5jpc=; b=W3xpCScQ10r4weTwIQQi64XQXOxTz1NmXE0Apxa1bYO0UhNv74+RKTsw8UmmrM3Ir3+HoX /42zP7UVJOxG8wDA== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 601E12C141; Wed, 21 Jun 2023 10:36:48 +0000 (UTC) Date: Wed, 21 Jun 2023 10:36:48 +0000 (UTC) From: Richard Biener To: Richard Sandiford cc: Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] tree-optimization/110243 - kill off IVOPTs split_offset In-Reply-To: Message-ID: References: <20230616123424.B38AC1330B@imap2.suse-dmz.suse.de> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-4.9 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 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 Wed, 21 Jun 2023, Richard Biener wrote: > On Tue, 20 Jun 2023, Richard Sandiford wrote: > > > Richard Biener writes: > > > On Mon, 19 Jun 2023, Richard Sandiford wrote: > > > > > >> Jeff Law writes: > > >> > On 6/16/23 06:34, Richard Biener via Gcc-patches wrote: > > >> >> IVOPTs has strip_offset which suffers from the same issues regarding > > >> >> integer overflow that split_constant_offset did but the latter was > > >> >> fixed quite some time ago. The following implements strip_offset > > >> >> in terms of split_constant_offset, removing the redundant and > > >> >> incorrect implementation. > > >> >> > > >> >> The implementations are not exactly the same, strip_offset relies > > >> >> on ptrdiff_tree_p to fend off too large offsets while split_constant_offset > > >> >> simply assumes those do not happen and truncates them. By > > >> >> the same means strip_offset also handles POLY_INT_CSTs but > > >> >> split_constant_offset does not. Massaging the latter to > > >> >> behave like strip_offset in those cases might be the way to go? > > >> >> > > >> >> Bootstrapped and tested on x86_64-unknown-linux-gnu. > > >> >> > > >> >> Comments? > > >> >> > > >> >> Thanks, > > >> >> Richard. > > >> >> > > >> >> PR tree-optimization/110243 > > >> >> * tree-ssa-loop-ivopts.cc (strip_offset_1): Remove. > > >> >> (strip_offset): Make it a wrapper around split_constant_offset. > > >> >> > > >> >> * gcc.dg/torture/pr110243.c: New testcase. > > >> > Your call -- IMHO you know this code far better than I. > > >> > > >> +1, but LGTM FWIW. I couldn't see anything obvious (and valid) > > >> that split_offset_1 handles and split_constant_offset doesn't. > > > > > > I think it's only the INTEGER_CST vs. ptrdiff_tree_p where the > > > latter (used in split_offset_1) handles POLY_INT_CSTs. split_offset > > > also computes the offset in poly_int64 and checks it fits > > > (to some extent) while split_constant_offset simply converts all > > > INTEGER_CSTs to ssizetype because it knows it starts from addresses > > > only. > > > > > > An alternative fix would have been to rewrite signed arithmetic > > > to unsigned in strip_offset_1. > > > > > > I wonder if we want to change split_constant_offset to record the > > > offset in a poly_int64 and have a wrapper converting it back to > > > a tree for data-ref analysis. > > > > Sounds a good idea if it's easily doable. > > > > > Then we can at least put cst_and_fits_in_hwi checks in the code? > > > > What would they be protecting against, if we're dealing with > > address arithmetic? > > While tree-data-ref.cc deals with address arithmetic only IVOPTs > at least also runs it on general IVs, for example for uses > in the exit condition. > > Adding the following to strip_offset > > gcc_assert (POINTER_TYPE_P (TREE_TYPE (expr)) > || (INTEGRAL_TYPE_P (TREE_TYPE (expr)) > && TYPE_PRECISION (TREE_TYPE (expr)) <= TYPE_PRECISION > (sizetype))); > > runs into ICEs when testing a 32bit target. > > But IVOPTs only makes use of the computed offset when it strips > it off address uses. But what I only now realized is that > IVOPTs strip_offset is also used by loop distribution. > > I'm going to split the patch in two at least to make these things > more obvious before changing the implementation. Hmm, but still split_constant_offset for example does case MULT_EXPR: if (TREE_CODE (op1) != INTEGER_CST) return false; split_constant_offset (op0, &var0, &off0, &op0_range, cache, limit); op1_range.set (TREE_TYPE (op1), wi::to_wide (op1), wi::to_wide (op1)); *off = size_binop (MULT_EXPR, off0, fold_convert (ssizetype, op1)); if (!compute_distributive_range (type, op0_range, code, op1_range, off, result_range)) return false; *var = fold_build2 (MULT_EXPR, sizetype, var0, fold_convert (sizetype, op1)); so *var is affected as well since we might truncate op1 here. In fact we at the end do if (INTEGRAL_TYPE_P (type)) *var = fold_convert (sizetype, *var); so truncate things (the API is documented to do that). The issue in the PR the change is fixing is that we end up with an expression that overflows but uses signed arithmetic and so we miscompile it later. IIRC the fixes to split_constant_offset always were that the sum of the base + offset wasn't equal to the original expression, right? Richard.