From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1872 invoked by alias); 27 Nov 2012 11:44:34 -0000 Received: (qmail 1858 invoked by uid 22791); 27 Nov 2012 11:44:33 -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; Tue, 27 Nov 2012 11:44:24 +0000 Received: (qmail 35795 invoked by uid 20157); 27 Nov 2012 11:44:23 -0000 Received: from localhost (sendmail-bs@127.0.0.1) by localhost with SMTP; 27 Nov 2012 11:44:23 -0000 Date: Tue, 27 Nov 2012 11:44:00 -0000 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 Content-Type: TEXT/PLAIN; charset=US-ASCII X-IsSubscribed: yes 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/msg02186.txt.bz2 I quoted the whole discussion, see a single line below. On Mon, 19 Nov 2012, Eric Botcazou wrote: > > 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. IIIUC, Jakub disagrees on this point, see . Sigh, hoping we can get consensus on this point, or at least that gcc has a consistent view... > > 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