[RFA:] fix PR55030, wrong code from __builtin_setjmp_receiver

[Eric Botcazou]

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: p.diff
Type: text/x-patch
Size: 2027 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20121119/058114cc/attachment.bin>