From patchwork Mon Nov 26 03:30:25 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans-Peter Nilsson X-Patchwork-Id: 201623 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id CC8E02C0082 for ; Mon, 26 Nov 2012 14:30:37 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1354505439; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Date:From:To:cc:Subject:In-Reply-To:Message-ID:References: User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=fmPhnduW881YMiNzEkhiMnXEVY0=; b=hFHvB52lopAzAjk yrwbH2pws8C6bFeQCcKxZKik9Jn91NFcWBn6KMmI5N/9kfyYayOLIu4+7QslZsbs Vq3R8PapvCzdYQK0GY260l8UOMKWJwj+KueJbkvvgpdIkR0Y6XEIyuEkSXDDdV/k cflVy1BNIey4JnIhfqWwX/VwbRfw= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Date:From:To:cc:Subject:In-Reply-To:Message-ID:References:User-Agent:MIME-Version:Content-Type:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=xkefhVnLvdU5P19W4tXYpHbeM79zs07ebn7hNCpnAhA03HDmzd6pY/K0xOtCRQ R+qBt7ifd23ikVjrveuMzeNFAI6j3qAznKWmewDJbI5FRVES9xKuSo/pdC3lRFI2 QWlDEi8cCozgZQrsVV4Z6yDdfzKNValbdMdT8mxo4IH54=; Received: (qmail 903 invoked by alias); 26 Nov 2012 03:30:32 -0000 Received: (qmail 894 invoked by uid 22791); 26 Nov 2012 03:30:32 -0000 X-SWARE-Spam-Status: No, hits=-2.9 required=5.0 tests=AWL, BAYES_00, KHOP_THREADED X-Spam-Check-By: sourceware.org Received: from dair.pair.com (HELO dair.pair.com) (209.68.1.49) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Mon, 26 Nov 2012 03:30:27 +0000 Received: (qmail 65040 invoked by uid 20157); 26 Nov 2012 03:30:25 -0000 Received: from localhost (sendmail-bs@127.0.0.1) by localhost with SMTP; 26 Nov 2012 03:30:25 -0000 Date: Sun, 25 Nov 2012 22:30:25 -0500 (EST) From: Hans-Peter Nilsson To: Eric Botcazou cc: gcc-patches@gcc.gnu.org, Alexandre Oliva , Jakub Jelinek Subject: Re: [RFA:] fix PR55030, wrong code from __builtin_setjmp_receiver In-Reply-To: <2166177.TtGqA2rZBR@polaris> Message-ID: References: <13011180.NBQR3vZSIa@polaris> <2166177.TtGqA2rZBR@polaris> User-Agent: Alpine 2.02 (BSF 1266 2009-07-14) MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Thanks for the reviews! On Mon, 19 Nov 2012, Eric Botcazou wrote: > Thanks for the analysis. I don't think that the redundancy of the clobber > list matters here: the clobbers are added to all asm statements, volatile or > not, so they aren't intended to make the volatile statements more precise in > the hope of optimizing around them. Oh, right. > > I'm not sure what to do with this. Try changing volatile_insn_p > > adding a parameter to optionally allow volatile asms with > > outputs to pass? But then, when *should* that distinction be > > done, to let such volatile asms be allowed to pass as > > not-really-as-volatile-as-we-look-for? I'd think "never" for > > any of the the patched code, or maybe "only when looking at > > effects on memory". > > Yes, weakening volatile_insn_p seems also dangerous to me, and I'd rather lean > towards the opposite, conservative side. > > We apparently have a small conflict between the meaning of volatile asms with > operands at the source level and volatile_insn_p. However, I think that the > latter interpretation is the correct one: if your asm statements have effects > that can be described, then you should use output constraints instead of > volatile; otherwise, you should use volatile and the output constraints have > essentially no meaning. > > The gcc.dg/guality/pr36728-[12].c testcases use volatile asms in an artificial > way to coax the compiler into following a certain behaviour, so I don't think > that they should be taken into account to judge the correctness of the change. > Therefore, I'd go ahead and apply the full patch below, possibly adjusting > both testcases to cope with it. Tentative (and untested) patch attached. > > I've also CCed Jakub though, as he might have a different opinion. Ok, no comments, so let's go with adjusting the test-cases. But, your patch for the test-cases doesn't work with (or without) the patch for pr36728-2.c. Bonus points for being very close, though! I don't really see *why* your patch shouldn't work. Specifically, I don't see why the *wrong value* is displayed instead of at least "" for any changed behavior, so I entered PR55467 for the related observations. By instead of your approach; not using x[0] after the asm, and instead changing the asm to also write the result to an external variable (i.e. an unknown operation writing there and x[0], with x[0] as input), I think I mimicked the intended behavior, such that x[0] is preserved as a variable until the line with the asm. At least nothing changes for -O1 besides debug info and the write to the variable "a" in main, for an unpatched gcc with/without patched test-case. (The patched test-cases all pass for an otherwise unpatched gcc at r193777.) Committed after committing the reapplied previous patch; r193802 and r193803. > > gcc: > > PR middle-end/55030 > > * builtins.c (expand_builtin_setjmp_receiver): Update comment > > regarding purpose of blockage. > > * emit-rtl.c [!HAVE_blockage] (gen_blockage): Similarly for > > the head comment. > > * rtlanal.c (volatile_insn_p): Ditto. > > * doc/md.texi (blockage): Update similarly. Change wording to > > require one of two forms, rather than implying a wider choice. > > * cse.c (cse_insn): Where checking for blocking insns, use > > volatile_insn_p instead of manual check for volatile ASM. > > * dse.c (scan_insn): Ditto. > > * cselib.c (cselib_process_insn): Ditto. testsuite: PR middle-end/55030 * gcc.dg/guality/pr36728-1.c, gcc.dg/guality/pr36728-2.c (foo): Don't use volatile asms, use plain asms. Where the output value for the asm is unused, write a global variable. Index: gcc/testsuite/gcc.dg/guality/pr36728-1.c =================================================================== --- gcc/testsuite/gcc.dg/guality/pr36728-1.c (revision 193776) +++ gcc/testsuite/gcc.dg/guality/pr36728-1.c (working copy) @@ -1,7 +1,7 @@ /* PR debug/36728 */ /* { dg-do run } */ /* { dg-options "-g" } */ - +int a; int __attribute__((noinline)) foo (int arg1, int arg2, int arg3, int arg4, int arg5, int arg6, int arg7) { @@ -9,9 +9,9 @@ foo (int arg1, int arg2, int arg3, int a int __attribute__ ((aligned(32))) y; y = 2; - asm volatile ("" : "=m" (y) : "m" (y)); + asm ("" : "=m" (y) : "m" (y)); x[0] = 25; - asm volatile ("" : "=m" (x[0]) : "m" (x[0])); + asm ("" : "=m" (x[0]), "=m" (a) : "m" (x[0])); return y; } @@ -43,7 +43,7 @@ int main () { int l = 0; - asm volatile ("" : "=r" (l) : "0" (l)); - foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30); + asm ("" : "=r" (l) : "0" (l)); + a = foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30); return 0; } Index: gcc/testsuite/gcc.dg/guality/pr36728-2.c =================================================================== --- gcc/testsuite/gcc.dg/guality/pr36728-2.c (revision 193776) +++ gcc/testsuite/gcc.dg/guality/pr36728-2.c (working copy) @@ -1,7 +1,7 @@ /* PR debug/36728 */ /* { dg-do run } */ /* { dg-options "-g" } */ - +int a; int __attribute__((noinline)) foo (int arg1, int arg2, int arg3, int arg4, int arg5, int arg6, int arg7) { @@ -9,9 +9,9 @@ foo (int arg1, int arg2, int arg3, int a int __attribute__ ((aligned(32))) y; y = 2; - asm volatile ("" : "=m" (y) : "m" (y)); + asm ("" : "=m" (y) : "m" (y)); x[0] = 25; - asm volatile ("" : "=m" (x[0]) : "m" (x[0])); + asm ("" : "=m" (x[0]), "=m" (a) : "m" (x[0])); return y; } @@ -43,7 +43,7 @@ int main () { int l = 0; - asm volatile ("" : "=r" (l) : "0" (l)); - foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30); + asm ("" : "=r" (l) : "0" (l)); + a = foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30); return 0; }