Message ID | 20220513102302.1FBE213446@imap2.suse-dmz.suse.de |
---|---|
State | Committed |
Commit | 060173dd73fcaf0767215f9d989ad064e2d5fe2a |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4CE4A3838006 for <patchwork@sourceware.org>; Fri, 13 May 2022 10:23:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 4CE4A3838006 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1652437412; bh=oesUI+DmzlJw3qxzS0vvX4VtbOq5ZJUDJOGrnd7/Lp8=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=MGRFa1hj6MovSL4HoifQLCkX+SGA3AcIE/OogvoIB4zv2BHt22vQ022YxFnwxXyW1 0neG+tveDpmi9zxN3G+UiUgsx73Ej+unQgISa6ftL0ZV+JeXk5MZlAuxrmKG7hQL+q O+hv/tCbCingyZb4gl4y4YKvV1ykyzFlkzymjPG4= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 4453B3839C47 for <gcc-patches@gcc.gnu.org>; Fri, 13 May 2022 10:23:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4453B3839C47 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 370A421AA8; Fri, 13 May 2022 10:23:02 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 1FBE213446; Fri, 13 May 2022 10:23:02 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id 2YGdBoYxfmJMGQAAMHmgww (envelope-from <rguenther@suse.de>); Fri, 13 May 2022 10:23:02 +0000 Date: Fri, 13 May 2022 12:23:01 +0200 (CEST) To: gcc-patches@gcc.gnu.org Subject: [PATCH] Fix OMP CAS expansion with separate condition MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Message-Id: <20220513102302.1FBE213446@imap2.suse-dmz.suse.de> X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Richard Biener <rguenther@suse.de> Cc: Jakub Jelinek <jakub@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Fix OMP CAS expansion with separate condition
|
|
Commit Message
Richard Biener
May 13, 2022, 10:23 a.m. UTC
When forcing the condition to be split out from COND_EXPRs I see a runtime failure of libgomp.fortran/atomic-19.f90 which can be reduced to !$omp atomic update, compare, capture if (x == 69_2 - r) x = 6_8 v = x being miscompiled, the difference being - _13 = .ATOMIC_COMPARE_EXCHANGE (_9, _10, _11, 4, 0, 0); - _14 = IMAGPART_EXPR <_13>; - _15 = REALPART_EXPR <_13>; - _16 = _14 != 0 ? _11 : _15; - _2 = (integer(kind=4)) _16; - v_17 = _2; + _14 = .ATOMIC_COMPARE_EXCHANGE (_10, _11, _12, 4, 0, 0); + _15 = IMAGPART_EXPR <_14>; + _16 = REALPART_EXPR <_14>; + _2 = (logical(kind=1)) _15; + _3 = (integer(kind=4)) _16; + v_17 = _3; where one can see a missing COND_EXPR. It seems to be a latent issue to me given the code can be exercised, it just maybe misses a 'need_new' testcase combined with 'cond_stmt'. Appearantly the if (cond_stmt) code is just to avoid creating a temporary (and possibly to preserve the condition compute if used elsewhere since the original stmt is going to be deleted). The following makes the failure go away for me in my patched tree and it also survives libgomp and gomp testing in an unpatched tree. Full bootstrap & regtest running on x86_64-unknown-linux-gnu. OK? Thanks, Richard. 2022-05-13 Richard Biener <rguenther@suse.de> * omp-expand.cc (expand_omp_atomic_cas): Do not short-cut computation of the new value. --- gcc/omp-expand.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
Comments
On Fri, May 13, 2022 at 12:23:01PM +0200, Richard Biener wrote: > 2022-05-13 Richard Biener <rguenther@suse.de> > > * omp-expand.cc (expand_omp_atomic_cas): Do not short-cut > computation of the new value. Ok, thanks. Though, depending on what exactly you allow or disallow, maybe even the im != 0 might not be acceptable. Oh, and if COND_EXPRs can only use some limited set of comparisons, we might need to adjust e.g. arith_overflow_check_p and various other spots in tree-ssa-math-opts.cc and other passes. > diff --git a/gcc/omp-expand.cc b/gcc/omp-expand.cc > index ee708314793..9fcc67a3448 100644 > --- a/gcc/omp-expand.cc > +++ b/gcc/omp-expand.cc > @@ -9092,16 +9092,17 @@ expand_omp_atomic_cas (basic_block load_bb, tree addr, > > if (cond_stmt) > { > - g = gimple_build_assign (gimple_assign_lhs (cond_stmt), > - NOP_EXPR, im); > + g = gimple_build_assign (cond, NOP_EXPR, im); > gimple_set_location (g, loc); > gsi_insert_before (&gsi, g, GSI_SAME_STMT); > } > - else if (need_new) > + > + if (need_new) > { > g = gimple_build_assign (create_tmp_reg (itype), COND_EXPR, > - build2 (NE_EXPR, boolean_type_node, > - im, build_zero_cst (itype)), > + cond_stmt > + ? cond : build2 (NE_EXPR, boolean_type_node, > + im, build_zero_cst (itype)), > d, re); > gimple_set_location (g, loc); > gsi_insert_before (&gsi, g, GSI_SAME_STMT); > -- > 2.35.3 Jakub
On Thu, 19 May 2022, Jakub Jelinek wrote: > On Fri, May 13, 2022 at 12:23:01PM +0200, Richard Biener wrote: > > 2022-05-13 Richard Biener <rguenther@suse.de> > > > > * omp-expand.cc (expand_omp_atomic_cas): Do not short-cut > > computation of the new value. > > Ok, thanks. > > Though, depending on what exactly you allow or disallow, maybe even > the im != 0 might not be acceptable. > Oh, and if COND_EXPRs can only use some limited set of comparisons, we might > need to adjust e.g. arith_overflow_check_p and various other spots in > tree-ssa-math-opts.cc and other passes. With the changes I have in the queue the !cond_stmt path should be effectively dead code (though I don't remove such immediately). Not sure what you think the issue with arith_overflow_check_p would be? Note this is only about COND_EXPRs in gimple assignments. I'm going to push this OMP patch later but will delay pushing the COND_EXPR patches until next week so I'm around more reliably to deal with fallout. Richard. > > diff --git a/gcc/omp-expand.cc b/gcc/omp-expand.cc > > index ee708314793..9fcc67a3448 100644 > > --- a/gcc/omp-expand.cc > > +++ b/gcc/omp-expand.cc > > @@ -9092,16 +9092,17 @@ expand_omp_atomic_cas (basic_block load_bb, tree addr, > > > > if (cond_stmt) > > { > > - g = gimple_build_assign (gimple_assign_lhs (cond_stmt), > > - NOP_EXPR, im); > > + g = gimple_build_assign (cond, NOP_EXPR, im); > > gimple_set_location (g, loc); > > gsi_insert_before (&gsi, g, GSI_SAME_STMT); > > } > > - else if (need_new) > > + > > + if (need_new) > > { > > g = gimple_build_assign (create_tmp_reg (itype), COND_EXPR, > > - build2 (NE_EXPR, boolean_type_node, > > - im, build_zero_cst (itype)), > > + cond_stmt > > + ? cond : build2 (NE_EXPR, boolean_type_node, > > + im, build_zero_cst (itype)), > > d, re); > > gimple_set_location (g, loc); > > gsi_insert_before (&gsi, g, GSI_SAME_STMT); > > -- > > 2.35.3 > > Jakub > >
On Thu, May 19, 2022 at 02:13:33PM +0000, Richard Biener wrote: > > Though, depending on what exactly you allow or disallow, maybe even > > the im != 0 might not be acceptable. > > Oh, and if COND_EXPRs can only use some limited set of comparisons, we might > > need to adjust e.g. arith_overflow_check_p and various other spots in > > tree-ssa-math-opts.cc and other passes. > > With the changes I have in the queue the !cond_stmt path should be > effectively dead code (though I don't remove such immediately). > > Not sure what you think the issue with arith_overflow_check_p would be? > Note this is only about COND_EXPRs in gimple assignments. Just that there will be more dead code in the compiler. At least tree-ssa-math-opts.cc in various spots (but I think phiopt too and other spots I've touched too) are looking for all 3 kinds of comparisons, GIMPLE_CONDs, comparisons in COND_EXPR first operand and assignments with tcc_comparison rhs codes and deal with all 3. If we restrict what can appear in COND_EXPR first operand in gimple assignments, perhaps we'll need to only deal with the first and last case and not the COND_EXPR ones. Jakub
On Thu, 19 May 2022, Jakub Jelinek wrote: > On Thu, May 19, 2022 at 02:13:33PM +0000, Richard Biener wrote: > > > Though, depending on what exactly you allow or disallow, maybe even > > > the im != 0 might not be acceptable. > > > Oh, and if COND_EXPRs can only use some limited set of comparisons, we might > > > need to adjust e.g. arith_overflow_check_p and various other spots in > > > tree-ssa-math-opts.cc and other passes. > > > > With the changes I have in the queue the !cond_stmt path should be > > effectively dead code (though I don't remove such immediately). > > > > Not sure what you think the issue with arith_overflow_check_p would be? > > Note this is only about COND_EXPRs in gimple assignments. > > Just that there will be more dead code in the compiler. > At least tree-ssa-math-opts.cc in various spots (but I think phiopt too and > other spots I've touched too) are looking for all 3 kinds of comparisons, > GIMPLE_CONDs, comparisons in COND_EXPR first operand and assignments > with tcc_comparison rhs codes and deal with all 3. If we restrict what > can appear in COND_EXPR first operand in gimple assignments, perhaps we'll > need to only deal with the first and last case and not the COND_EXPR ones. Ah, yeah - there will be followup cleanup opportunities like shaving off 5% of gimple-match.cc when I'm happy with a required phiopt adjustment. Richard.
diff --git a/gcc/omp-expand.cc b/gcc/omp-expand.cc index ee708314793..9fcc67a3448 100644 --- a/gcc/omp-expand.cc +++ b/gcc/omp-expand.cc @@ -9092,16 +9092,17 @@ expand_omp_atomic_cas (basic_block load_bb, tree addr, if (cond_stmt) { - g = gimple_build_assign (gimple_assign_lhs (cond_stmt), - NOP_EXPR, im); + g = gimple_build_assign (cond, NOP_EXPR, im); gimple_set_location (g, loc); gsi_insert_before (&gsi, g, GSI_SAME_STMT); } - else if (need_new) + + if (need_new) { g = gimple_build_assign (create_tmp_reg (itype), COND_EXPR, - build2 (NE_EXPR, boolean_type_node, - im, build_zero_cst (itype)), + cond_stmt + ? cond : build2 (NE_EXPR, boolean_type_node, + im, build_zero_cst (itype)), d, re); gimple_set_location (g, loc); gsi_insert_before (&gsi, g, GSI_SAME_STMT);