Patchwork Fix volatile aggregate x = x; (PR middle-end/45852) (take 2)

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 20, 2010, 10:29 p.m.
Message ID <20101220222944.GC16156@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/76240/
State New
Headers show

Comments

Jakub Jelinek - Dec. 20, 2010, 10:29 p.m.
On Sun, Dec 19, 2010 at 06:56:58PM -0800, Mark Mitchell wrote:
> On 12/15/2010 9:59 AM, Jakub Jelinek wrote:
> 
> > CCing Mark who has added alt_rtl stuff.
> 
> Unfortunately, I remember nothing about this.  I went and looked at the
> code, and I just don't fully recall what was going on there.
> 
> > I think either we just shouldn't set alt_rtl at all when it has side effects
> > (as done in this patch), or the above if should do similar testing on
> > side_effects_p for alt_rtl/target.
> 
> I suspect the latter.  I think that it's more logical for the low-level
> function (expand_expr_real_1) to say "this is what's being modified" and
> for the high-level one to say "OK, but I don't care" than it is for the
> low-level function to try to guess why the high-level one wants the
> information.
> 
> I suppose that's just style, though, not substance.

Here it is, again bootstrapped/regtested on x86_64-linux and i686-linux, ok?

2010-12-20  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/45852
	* expr.c (store_expr): Ignore alt_rtl if equal to target,
	but has side-effects.

	* gcc.target/i386/pr45852.c: New test.


	Jakub
Mark Mitchell - Dec. 20, 2010, 10:37 p.m.
On 12/20/2010 2:29 PM, Jakub Jelinek wrote:

> 2010-12-20  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/45852
> 	* expr.c (store_expr): Ignore alt_rtl if equal to target,
> 	but has side-effects.
> 
> 	* gcc.target/i386/pr45852.c: New test.

This looks good to me.

Thank you,

Patch

--- gcc/expr.c.jj	2010-12-16 22:42:37.770233358 +0100
+++ gcc/expr.c	2010-12-20 21:12:28.883913714 +0100
@@ -4712,7 +4712,10 @@  store_expr (tree exp, rtx target, int ca
       /* If store_expr stores a DECL whose DECL_RTL(exp) == TARGET,
 	 but TARGET is not valid memory reference, TEMP will differ
 	 from TARGET although it is really the same location.  */
-      && !(alt_rtl && rtx_equal_p (alt_rtl, target))
+      && !(alt_rtl
+	   && rtx_equal_p (alt_rtl, target)
+	   && !side_effects_p (alt_rtl)
+	   && !side_effects_p (target))
       /* If there's nothing to copy, don't bother.  Don't call
 	 expr_size unless necessary, because some front-ends (C++)
 	 expr_size-hook must not be given objects that are not
--- gcc/testsuite/gcc.target/i386/pr45852.c.jj	2010-12-20 20:59:41.079868716 +0100
+++ gcc/testsuite/gcc.target/i386/pr45852.c	2010-12-20 20:59:41.079868716 +0100
@@ -0,0 +1,16 @@ 
+/* PR middle-end/45852 */
+/* { dg-options "-O2 -mcmodel=small" } */
+/* { dg-do compile { target { { i?86-*-linux* x86_64-*-linux* } && lp64 } } } */
+/* { dg-require-visibility "" } */
+
+struct S { int s; };
+
+volatile struct S globvar __attribute__((visibility ("hidden"))) = { -6 };
+
+void
+foo (void)
+{
+  globvar = globvar;
+}
+
+/* { dg-final { scan-assembler-times "globvar.%?rip" 2 } } */