From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1964 invoked by alias); 19 Nov 2012 09:35:01 -0000 Received: (qmail 1862 invoked by uid 22791); 19 Nov 2012 09:35:00 -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 mel.act-europe.fr (HELO mel.act-europe.fr) (194.98.77.210) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 19 Nov 2012 09:34:52 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-smtp.eu.adacore.com (Postfix) with ESMTP id 80646CB17CC; Mon, 19 Nov 2012 10:34:53 +0100 (CET) Received: from mel.act-europe.fr ([127.0.0.1]) by localhost (smtp.eu.adacore.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id COAiiHkHIkje; Mon, 19 Nov 2012 10:34:53 +0100 (CET) Received: from polaris.localnet (bon31-6-88-161-99-133.fbx.proxad.net [88.161.99.133]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mel.act-europe.fr (Postfix) with ESMTP id 2E3E5CB17C6; Mon, 19 Nov 2012 10:34:53 +0100 (CET) From: Eric Botcazou To: Hans-Peter Nilsson Cc: gcc-patches@gcc.gnu.org, Alexandre Oliva , Jakub Jelinek Subject: Re: [RFA:] fix PR55030, wrong code from __builtin_setjmp_receiver Date: Mon, 19 Nov 2012 09:35:00 -0000 Message-ID: <2166177.TtGqA2rZBR@polaris> User-Agent: KMail/4.7.2 (Linux/3.1.10-1.16-desktop; KDE/4.7.2; x86_64; ; ) In-Reply-To: References: <13011180.NBQR3vZSIa@polaris> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nextPart2710962.HsSpXZseTs" Content-Transfer-Encoding: 7Bit Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-11/txt/msg01542.txt.bz2 --nextPart2710962.HsSpXZseTs Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Content-length: 4251 > Unfortunately, it causes regressions; read on for a very brief > analysis. > > For x86_64-linux (base multilib): > > Running > /home/hp/gcctop/tmp/pr55030-2n/gcc/gcc/testsuite/gcc.dg/guality/guality.exp > ... ... > FAIL: gcc.dg/guality/pr36728-1.c -O1 line 12 arg7 == 30 > [...] > > I looked into gcc.dg/guality/pr36728-1.c at base -O1. > > The generated assembly code modulo debug info, is the same. The > value of arg7 is optimized out, says gdb in gcc.log. The > problem doesn't seem to be any md-generated > frame-related-barrier as was my first guess, but the volatile > asms in the source(!). The first one looks like this (and the > second one similar) in pr36728-1.c.168r.cse1 (r193583): > > (insn 26 25 27 2 (parallel [ > (set (mem/c:SI (plus:DI (reg/f:DI 20 frame) > (const_int -32 [0xffffffffffffffe0])) [0 y+0 S4 > A256]) (asm_operands/v:SI ("") ("=m") 0 [ > (mem/c:SI (plus:DI (reg/f:DI 20 frame) > (const_int -32 [0xffffffffffffffe0])) [0 y+0 > S4 A256]) ] > [ > (asm_input:SI ("m") (null):0) > ] > [] > /home/hp/gcctop/tmp/nbasf/gcc/gcc/testsuite/gcc.dg/guality/pr36728-1.c:12)) > (clobber (reg:QI 18 fpsr)) > (clobber (reg:QI 17 flags)) > ]) > /home/hp/gcctop/tmp/nbasf/gcc/gcc/testsuite/gcc.dg/guality/pr36728-1.c:12 > -1 (nil)) > > It's not caught by the previous test: > - && GET_CODE (PATTERN (insn)) == ASM_OPERANDS > - && MEM_VOLATILE_P (PATTERN (insn))) > > ...but since it is a volatile asm (as seen in the source and by > the /v on the asm_operands) volatile_insn_p does catch it (as > expected and intended) and down the road for some reason, gcc > can't recover the arg7 contents. Somewhat expected, but this > volatile asm is also more volatile than intended; a clobber list > for example as seen above inserted by the md, is now redundant. 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. > 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. > 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. -- Eric Botcazou --nextPart2710962.HsSpXZseTs Content-Disposition: attachment; filename="p.diff" Content-Transfer-Encoding: 7Bit Content-Type: text/x-patch; charset="utf-8"; name="p.diff" Content-length: 2027 Index: gcc.dg/guality/pr36728-1.c =================================================================== --- gcc.dg/guality/pr36728-1.c (revision 193596) +++ gcc.dg/guality/pr36728-1.c (working copy) @@ -9,10 +9,10 @@ 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])); - return y; + asm ("" : "=m" (x[0]) : "m" (x[0])); + return y + x[0]; } /* On s390(x) r2 and r3 are (depending on the optimization level) used @@ -39,11 +39,13 @@ foo (int arg1, int arg2, int arg3, int a /* { dg-final { gdb-test 14 "*x" "(char) 25" } } */ /* { dg-final { gdb-test 14 "y" "2" } } */ +int a; + 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.dg/guality/pr36728-2.c =================================================================== --- gcc.dg/guality/pr36728-2.c (revision 193596) +++ gcc.dg/guality/pr36728-2.c (working copy) @@ -9,10 +9,10 @@ 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])); - return y; + asm ("" : "=m" (x[0]) : "m" (x[0])); + return y + x[0]; } /* On s390(x) r2 and r3 are (depending on the optimization level) used @@ -39,11 +39,13 @@ foo (int arg1, int arg2, int arg3, int a /* { dg-final { gdb-test 14 "*x" "(char) 25" } } */ /* { dg-final { gdb-test 14 "y" "2" } } */ +int a; + 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; } --nextPart2710962.HsSpXZseTs--