Patchwork [RFA:] fix PR55030, wrong code from __builtin_setjmp_receiver

login
register
mail settings
Submitter Eric Botcazou
Date Nov. 19, 2012, 9:31 a.m.
Message ID <2166177.TtGqA2rZBR@polaris>
Download mbox | patch
Permalink /patch/199959/
State New
Headers show

Comments

Eric Botcazou - Nov. 19, 2012, 9:31 a.m.
> 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.
Hans-Peter Nilsson - Nov. 27, 2012, 11:44 a.m.
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

Patch

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;
 }