From: Hans-Peter Nilsson <hp@bitrange.com>
To: Eric Botcazou <ebotcazou@adacore.com>
Cc: gcc-patches@gcc.gnu.org, Alexandre Oliva <aoliva@redhat.com>,
Jakub Jelinek <jakub@redhat.com>
Subject: Re: [RFA:] fix PR55030, wrong code from __builtin_setjmp_receiver
Date: Tue, 27 Nov 2012 11:44:00 -0000 [thread overview]
Message-ID: <alpine.BSF.2.02.1211270637070.30315@dair.pair.com> (raw)
In-Reply-To: <2166177.TtGqA2rZBR@polaris>
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
<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55467#c13>.
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
next prev parent reply other threads:[~2012-11-27 11:44 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-12 4:49 Hans-Peter Nilsson
2012-11-12 8:28 ` Eric Botcazou
2012-11-19 3:54 ` Hans-Peter Nilsson
2012-11-19 9:35 ` Eric Botcazou
2012-11-26 3:30 ` Hans-Peter Nilsson
2012-11-27 11:44 ` Hans-Peter Nilsson [this message]
2012-11-27 12:04 ` Jakub Jelinek
2012-11-27 12:27 ` Hans-Peter Nilsson
2012-11-27 13:32 ` Jakub Jelinek
2012-11-27 17:39 ` Eric Botcazou
2012-11-27 18:20 ` Jakub Jelinek
2012-12-01 0:03 ` Eric Botcazou
2012-11-28 20:10 ` Richard Sandiford
2012-11-28 20:46 ` Richard Henderson
2012-11-28 13:44 ` Hans-Peter Nilsson
2012-11-28 13:56 ` Jakub Jelinek
2012-11-28 20:53 ` Richard Henderson
2012-11-27 19:30 Uros Bizjak
2012-11-28 9:50 ` Uros Bizjak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.BSF.2.02.1211270637070.30315@dair.pair.com \
--to=hp@bitrange.com \
--cc=aoliva@redhat.com \
--cc=ebotcazou@adacore.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).