Patchwork Unshare constants in the constant pool (PR target/42894)

login
register
mail settings
Submitter Jakub Jelinek
Date Jan. 27, 2011, 8:13 p.m.
Message ID <20110127201329.GP2724@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/80730/
State New
Headers show

Comments

Jakub Jelinek - Jan. 27, 2011, 8:13 p.m.
On Thu, Jan 27, 2011 at 09:48:00AM +0100, Jakub Jelinek wrote:
> On Wed, Jan 26, 2011 at 06:29:25PM -0700, Jeff Law wrote:
> > Well, one could easily argue that using the original expression in the
> > REG_EQUAL note is incorrect as well.  In fact, looking over all the
> > calls to set_unique_reg_note shows that most either copy the value or
> > generate a new one.
> > 
> > I think that means we actually want both the varasm.c and the expr.c
> > change to avoid incorrect sharing.
> 
> I think the reason why most of the set_unique_reg_note calls copy_rtx
> is that the pattern is usually kept in the insn (or in some other insn).
> In the emit_move_insn case it specifically tests that the pattern is not
> there (well, some weird target could embed it in some unspec or something),
> the reason I changed force_const_mem was to deal with any other uses of
> force_const_mem.
> 
> But I guess if you strongly prefer to have it in both places I can test a
> patch.

This passed bootstrap/regtest on x86_64-linux and i686-linux too:

2011-01-27  Jakub Jelinek  <jakub@redhat.com>

	PR target/42894
	* varasm.c (force_const_mem): Store copy of x in desc->constant
	instead of x itself.
	* expr.c (emit_move_insn): Add a copy of y_cst instead of y_cst
	itself into REG_EQUAL note.

	* gcc.dg/tls/pr42894.c: New test.


	Jakub
Jeff Law - Jan. 28, 2011, 6:13 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/27/11 13:13, Jakub Jelinek wrote:
> On Thu, Jan 27, 2011 at 09:48:00AM +0100, Jakub Jelinek wrote:
>> On Wed, Jan 26, 2011 at 06:29:25PM -0700, Jeff Law wrote:
>>> Well, one could easily argue that using the original expression in the
>>> REG_EQUAL note is incorrect as well.  In fact, looking over all the
>>> calls to set_unique_reg_note shows that most either copy the value or
>>> generate a new one.
>>>
>>> I think that means we actually want both the varasm.c and the expr.c
>>> change to avoid incorrect sharing.
>>
>> I think the reason why most of the set_unique_reg_note calls copy_rtx
>> is that the pattern is usually kept in the insn (or in some other insn).
>> In the emit_move_insn case it specifically tests that the pattern is not
>> there (well, some weird target could embed it in some unspec or something),
>> the reason I changed force_const_mem was to deal with any other uses of
>> force_const_mem.
>>
>> But I guess if you strongly prefer to have it in both places I can test a
>> patch.
> 
> This passed bootstrap/regtest on x86_64-linux and i686-linux too:
> 
> 2011-01-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/42894
> 	* varasm.c (force_const_mem): Store copy of x in desc->constant
> 	instead of x itself.
> 	* expr.c (emit_move_insn): Add a copy of y_cst instead of y_cst
> 	itself into REG_EQUAL note.
> 
> 	* gcc.dg/tls/pr42894.c: New test.
Let's go with this.  It should be the safest -- and if a port shows up
which assumes the pool entries are shared, we can certainly deal with it.

Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNQwc0AAoJEBRtltQi2kC7huIIAK/H18lSp/ZQj4xgtjqCsk8j
H2BUqVRzI1hWiVI3BkmUl6PW3Xl0Qa/cfbtIEljELgfZ/yxxKwmDrlFJfP1yCEJX
1CP1wO5ox73wN68+8Axhy6479C6cIurWCMs18oBNOGi0LLmUPYZgDgKjrzSX1r7w
VGtcv+V3UzRz56lQqDKblui55ZqeDMKXXq7pHhDw+qNebKncpQtZAZPeZhYKh/Ey
YKoIxQCqpVjEDapSRhOUkRwmxw9Y748EMooR2Zh8JiLzqx28wfbc2ZzfS/yT/vIG
yInJ5RKyMdfXtmXT3I/gjkxKDM+zpdQvMRj588tudUlHmTWBImXU34OqDl1QkHk=
=R1q2
-----END PGP SIGNATURE-----

Patch

--- gcc/varasm.c.jj	2011-01-25 12:58:41.000000000 +0100
+++ gcc/varasm.c	2011-01-26 14:07:50.635389932 +0100
@@ -3518,7 +3518,7 @@  force_const_mem (enum machine_mode mode,
   pool->offset &= ~ ((align / BITS_PER_UNIT) - 1);
 
   desc->next = NULL;
-  desc->constant = tmp.constant;
+  desc->constant = copy_rtx (tmp.constant);
   desc->offset = pool->offset;
   desc->hash = hash;
   desc->mode = mode;
--- gcc/expr.c.jj	2011-01-26 14:07:50.635389932 +0100
+++ gcc/expr.c	2011-01-27 17:05:04.936795133 +0100
@@ -3398,7 +3398,7 @@  emit_move_insn (rtx x, rtx y)
       && (set = single_set (last_insn)) != NULL_RTX
       && SET_DEST (set) == x
       && ! rtx_equal_p (y_cst, SET_SRC (set)))
-    set_unique_reg_note (last_insn, REG_EQUAL, y_cst);
+    set_unique_reg_note (last_insn, REG_EQUAL, copy_rtx (y_cst));
 
   return last_insn;
 }
--- gcc/testsuite/gcc.dg/tls/pr42894.c.jj	2011-01-26 16:29:13.765389223 +0100
+++ gcc/testsuite/gcc.dg/tls/pr42894.c	2011-01-26 16:31:46.830433380 +0100
@@ -0,0 +1,12 @@ 
+/* PR target/42894 */
+/* { dg-do compile } */
+/* { dg-options "-march=armv5te -mthumb" { target arm*-*-* } } */
+/* { dg-require-effective-target tls } */
+
+extern __thread int t;
+
+void
+foo (int a)
+{
+  t = a;
+}