From patchwork Fri Jun 21 20:51:18 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Roger Sayle X-Patchwork-Id: 1950980 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=nextmovesoftware.com header.i=@nextmovesoftware.com header.a=rsa-sha256 header.s=default header.b=KCR5o75f; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4W5V000nGsz1ydW for ; Sat, 22 Jun 2024 06:51:42 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B9F163830B73 for ; Fri, 21 Jun 2024 20:51:40 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from server.nextmovesoftware.com (server.nextmovesoftware.com [69.48.154.134]) by sourceware.org (Postfix) with ESMTPS id CAED23831380 for ; Fri, 21 Jun 2024 20:51:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CAED23831380 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=nextmovesoftware.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=nextmovesoftware.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org CAED23831380 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=69.48.154.134 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1719003084; cv=none; b=cjDpg8zfIxmQ8EUq8gZpVcLz/V+Nyky+qHqpahcXgM63+H8r9CBy2Cbz7LPHnnbHQFzdmLdMW0ANQkSPT61SoLWG2dPCEdJbai8g+IOPvAf1rm4XFug5Quw0tfPhMMgg2MIh5XRer2uVauklLK45e3ZlrEkRMVxb2m9X7WDvR0w= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1719003084; c=relaxed/simple; bh=FZpE681YGYXwJgrmlLk5gp2VGMatac/w+pHDojWNuQQ=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=q6uTRwb2cJge9Q6gFrmQ3ncsEjqHcBNJKLlWgc86uzAOEbXdSX3lq9nQU/DbJDJERfYfu7zlkAAinKs5UheB2jveYfnU1GCOQEjOV6F4kvFPWiXFAKUtOj20gt5U8A3pecwEfuIke+K29ru0kX3S0hvVUE4iX7adPItYLW2RTRs= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=nextmovesoftware.com; s=default; h=Content-Type:MIME-Version:Message-ID: Date:Subject:In-Reply-To:References:Cc:To:From:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=hltLsCHvzb9D9neCbqkYRGGTP11C6mJB4u4XzgGaT4Y=; b=KCR5o75fDB7+tXxxV9TI7tI9N5 EKmA2UGP/UhSMAyTPdXylo7OqsvQIRNik7UP6u8zlZqwq5RXNNRqn4cXVa8RQFlj2JM/eJVpMkky4 AvKvc8tGw8pBz2iuqH9ueyfa/HYgM+aiHPedSyA1q9096alyfJPhhfNmSktGXSOF3HVo5kvz7NopK Zf1DWvyfTWOWMxcehXe3q5q6UmCuwDLgsgNeZpEY06vCBrd1JMiMZC/+V9iDhe1pqDqVwr3zNXTBL O6ODbvfKGtOcL9/ZC3IwfKeUzx/jUO8wk9E1LFRgz/RzNmFh+8F5a9VH/dS7XIxGSaAz8HhOJwbll HT43vj6w==; Received: from [168.86.198.82] (port=64644 helo=Dell) by server.nextmovesoftware.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.97.1) (envelope-from ) id 1sKlE9-0000000ABa4-0d7B; Fri, 21 Jun 2024 16:51:21 -0400 From: "Roger Sayle" To: Cc: "'Richard Biener'" References: <003c01da994c$085826d0$19087470$@nextmovesoftware.com> In-Reply-To: Subject: [PATCH v2] PR tree-opt/113673: Avoid load merging when potentially trapping. Date: Fri, 21 Jun 2024 21:51:18 +0100 Message-ID: <01b801dac41c$c4abc210$4e034630$@nextmovesoftware.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AdrEGtph8ya3Bp64QbaeWSykWcvdwg== Content-Language: en-gb X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - server.nextmovesoftware.com X-AntiAbuse: Original Domain - gcc.gnu.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - nextmovesoftware.com X-Get-Message-Sender-Via: server.nextmovesoftware.com: authenticated_id: roger@nextmovesoftware.com X-Authenticated-Sender: server.nextmovesoftware.com: roger@nextmovesoftware.com X-Source: X-Source-Args: X-Source-Dir: X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Hi Richard, Thanks for the review and apologies for taking so long to get back to this. This revision implements your suggestions from early May, as found at https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650405.html This patch fixes PR tree-optimization/113673, a P2 ice-on-valid regression caused by load merging of (ptr[0]<<8)+ptr[1] when -ftrapv has been specified. When the operator is | or ^ this is safe, but for addition of signed integer types, a trap may be generated/required, so merging this idiom into a single non-trapping instruction is inappropriate, confusing the compiler by transforming a basic block with an exception edge into one without. This revision implements Richard Biener's feedback to add an early check for stmt_can_throw_internal (cfun, stmt) to prevent transforming in the presence of any statement that could trap, not just overflow on addition. The one other tweak included in this patch is to mark the local function find_bswap_or_nop_load as static ensuring that it isn't called from outside this file, and guaranteeing that it is dominated by stmt_can_throw_internal checking. This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and make -k check, both with and without --target_board=unix{-m32} with no new failures. Ok for mainline? 2024-06-21 Roger Sayle Richard Biener gcc/ChangeLog PR tree-optimization/113673 * gimple-ssa-store-merging.cc (find_bswap_or_nop_load): Make static. (find_bswap_or_nop_1): Avoid transformations (load merging) when stmt_can_throw_internal indicates that a statement can trap. gcc/testsuite/ChangeLog PR tree-optimization/113673 * g++.dg/pr113673.C: New test case. Thanks in advance, Roger --- > -----Original Message----- > From: Richard Biener > Sent: 02 May 2024 10:27 > Subject: Re: [PATCH] PR tree-opt/113673: Avoid load merging from potentially > trapping additions. > > On Sun, Apr 28, 2024 at 11:11 AM Roger Sayle > wrote: > > > > This patch fixes PR tree-optimization/113673, a P2 ice-on-valid > > regression caused by load merging of (ptr[0]<<8)+ptr[1] when -ftrapv > > has been specified. When the operator is | or ^ this is safe, but for > > addition of signed integer types, a trap may be generated/required, so > > merging this idiom into a single non-trapping instruction is > > inappropriate, confusing the compiler by transforming a basic block > > with an exception edge into one without. One fix is to be more > > selective for PLUS_EXPR than for BIT_IOR_EXPR or BIT_XOR_EXPR in > > gimple-ssa-store-merging.cc's > > find_bswap_or_nop_1 function. > > > > An alternate solution might be to notice that in this idiom the > > addition can't overflow, but that this detail wasn't apparent when > > exception edges were added to the CFG. In which case, it's safe to > > remove (or mark for > > removal) the problematic exceptional edge. Unfortunately updating the > > CFG is a part of the compiler that I'm less familiar with. > > > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap > > and make -k check, both with and without --target_board=unix{-m32} > > with no new failures. Ok for mainline? > > Instead of > > + case PLUS_EXPR: > + /* Don't perform load merging if this addition can trap. */ > + if (cfun->can_throw_non_call_exceptions > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs1)) > + && TYPE_OVERFLOW_TRAPS (TREE_TYPE (rhs1))) > + return NULL; > > please check stmt_can_throw_internal (cfun, stmt) - the find_bswap_or_no_load > call in the function suffers from the same issue, so this should probably be > checked before that call even. > > Thanks, > Richard. > > > > > 2024-04-28 Roger Sayle > > > > gcc/ChangeLog > > PR tree-optimization/113673 > > * gimple-ssa-store-merging.cc (find_bswap_or_nop_1) > PLUS_EXPR>: > > Don't perform load merging if a signed addition may trap. > > > > gcc/testsuite/ChangeLog > > PR tree-optimization/113673 > > * g++.dg/pr113673.C: New test case. > > diff --git a/gcc/gimple-ssa-store-merging.cc b/gcc/gimple-ssa-store-merging.cc index cb0cb5f..7dba4a7 100644 --- a/gcc/gimple-ssa-store-merging.cc +++ b/gcc/gimple-ssa-store-merging.cc @@ -363,7 +363,7 @@ init_symbolic_number (struct symbolic_number *n, tree src) the answer. If so, REF is that memory source and the base of the memory area accessed and the offset of the access from that base are recorded in N. */ -bool +static bool find_bswap_or_nop_load (gimple *stmt, tree ref, struct symbolic_number *n) { /* Leaf node is an array or component ref. Memorize its base and @@ -610,7 +610,9 @@ find_bswap_or_nop_1 (gimple *stmt, struct symbolic_number *n, int limit) gimple *rhs1_stmt, *rhs2_stmt, *source_stmt1; enum gimple_rhs_class rhs_class; - if (!limit || !is_gimple_assign (stmt)) + if (!limit + || !is_gimple_assign (stmt) + || stmt_can_throw_internal (cfun, stmt)) return NULL; rhs1 = gimple_assign_rhs1 (stmt); diff --git a/gcc/testsuite/g++.dg/pr113673.C b/gcc/testsuite/g++.dg/pr113673.C new file mode 100644 index 0000000..1148977 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr113673.C @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-Os -fnon-call-exceptions -ftrapv" } */ + +struct s { ~s(); }; +void +h (unsigned char *data, int c) +{ + s a1; + while (c) + { + int m = *data++ << 8; + m += *data++; + } +}