* [PATCH] Unshare constants in the constant pool (PR target/42894)
@ 2011-01-26 20:01 Jakub Jelinek
2011-01-27 1:48 ` Jeff Law
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2011-01-26 20:01 UTC (permalink / raw)
To: gcc-patches
Hi!
As discussed in the PR, if a non-shareable constant is force_const_mem
and the original expression is used elsewhere in the IL (e.g. in REG_EQUAL
or REG_EQUIV note), unshare_all_rtl doesn't unshare it, but modification of
the IL might result in changes of the constant in theory and if
avoid_constant_pool_reference result is then pushed somewhere else into the
IL, we get RTL sharing errors (as on arm on this testcase).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok if it passes
bootstrap/regtest also on arm?
2011-01-26 Jakub Jelinek <jakub@redhat.com>
PR target/42894
* varasm.c (force_const_mem): Store copy of x in desc->constant
instead of x itself.
* gcc.dg/tls/pr42894.c: New test.
--- 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/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;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Unshare constants in the constant pool (PR target/42894)
2011-01-26 20:01 [PATCH] Unshare constants in the constant pool (PR target/42894) Jakub Jelinek
@ 2011-01-27 1:48 ` Jeff Law
2011-01-27 9:26 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Law @ 2011-01-27 1:48 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 01/26/11 12:37, Jakub Jelinek wrote:
> Hi!
>
> As discussed in the PR, if a non-shareable constant is force_const_mem
> and the original expression is used elsewhere in the IL (e.g. in REG_EQUAL
> or REG_EQUIV note), unshare_all_rtl doesn't unshare it, but modification of
> the IL might result in changes of the constant in theory and if
> avoid_constant_pool_reference result is then pushed somewhere else into the
> IL, we get RTL sharing errors (as on arm on this testcase).
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.
jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iQEcBAEBAgAGBQJNQMp1AAoJEBRtltQi2kC7LW4H/2MsJOnqPuZx+vKT5+4DCBYT
dwq6/4buU/Vbr3JOMHxNg+9FFKpC3s2gwl9N1JcsFPvBZw5XOgTuLZXR2KHoL4ub
xhF4Of1UaDIPsVZvxJnCA905ArHbIxKVELHgitWcvyIkGcJXV9u2/XXWBy27H5Ie
QvSc0ojBmEulqPnzI4r2UBlAFgKyix3OPEiZP/k8LEXAidVsezl4tB1T+kba6NtY
26Aq6mLiJ8kNQJIUjlv1I8a/pt9dMTP+Begvclro7x178caPIts+qhbucwNSHCf8
6jZbt6tm5JWUz87tzLtzC1/4rGvMuJXdpyvYUc7ul3LCe9KUNHGGxTbF359jRus=
=Vzz3
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Unshare constants in the constant pool (PR target/42894)
2011-01-27 1:48 ` Jeff Law
@ 2011-01-27 9:26 ` Jakub Jelinek
2011-01-27 20:47 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2011-01-27 9:26 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
On Wed, Jan 26, 2011 at 06:29:25PM -0700, Jeff Law wrote:
> > As discussed in the PR, if a non-shareable constant is force_const_mem
> > and the original expression is used elsewhere in the IL (e.g. in REG_EQUAL
> > or REG_EQUIV note), unshare_all_rtl doesn't unshare it, but modification of
> > the IL might result in changes of the constant in theory and if
> > avoid_constant_pool_reference result is then pushed somewhere else into the
> > IL, we get RTL sharing errors (as on arm on this testcase).
> 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.
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Unshare constants in the constant pool (PR target/42894)
2011-01-27 9:26 ` Jakub Jelinek
@ 2011-01-27 20:47 ` Jakub Jelinek
2011-01-28 19:40 ` Jeff Law
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2011-01-27 20:47 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
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.
--- 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;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Unshare constants in the constant pool (PR target/42894)
2011-01-27 20:47 ` Jakub Jelinek
@ 2011-01-28 19:40 ` Jeff Law
0 siblings, 0 replies; 5+ messages in thread
From: Jeff Law @ 2011-01-28 19:40 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
-----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-----
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-01-28 18:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-26 20:01 [PATCH] Unshare constants in the constant pool (PR target/42894) Jakub Jelinek
2011-01-27 1:48 ` Jeff Law
2011-01-27 9:26 ` Jakub Jelinek
2011-01-27 20:47 ` Jakub Jelinek
2011-01-28 19:40 ` Jeff Law
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).