| Message ID | 53CF77F5.4090904@arm.com |
|---|---|
| State | New |
| Headers | show |
On 07/23/14 02:53, Kyrill Tkachov wrote: > Darn, had forgotten to attach the patch... > > > On 16/07/14 12:30, Kyrill Tkachov wrote: >> Hi all, >> >> This fixes the PR mentioned in the subject. When expanding >> atomic_test_and_set we try the corresponding sync optabs and if none >> exist, like for pre-SMP ARM architectures (-march=armv6 and earlier) the >> code in optabs.c reverts to a single-threaded implementation that just >> does a load and store. >> >> However, if the result of the operation is to be ignored the code in >> builtins.c follows the convention that the target RTX is set to >> const0_rtx to signify that the result is ignored and the expand >> functions in optabs.c are supposed to check for that and act >> appropriately. >> >> The code in the single-threaded codepath in expand_atomic_test_and_set >> in optabs.c didn't perform this check and instead tried to emit a move >> to a const0_rtx which ICEs further down the line. >> >> This patch fixes that by checking if the result is ignored, and if it is >> omits the load. >> I wouldn't dare to remove the load in normal atomic handling code due to >> all the memory ordering subtleties, but this code path occurs only when >> we have a trivially single-threaded bare-metal target and the compiler >> assumes no races anyway and no dodgy context switches and tries to >> implement this with a ldrb+strb, so I think removing the ldrb is valid. >> >> Bootstrapped on arm, aarch64 and x86 and tested as well. >> >> Ok for trunk? >> This appears on 4.9 as well, I'll test it on that branch as well >> >> 2014-07-16 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/61713 >> * gcc/optabs.c (expand_atomic_test_and_set): Do not try to emit >> move to subtarget in serial version if result is ignored. OK. jeff
On 25/07/14 23:05, Jeff Law wrote: > On 07/23/14 02:53, Kyrill Tkachov wrote: >> Darn, had forgotten to attach the patch... >> >> >> On 16/07/14 12:30, Kyrill Tkachov wrote: >>> Hi all, >>> >>> This fixes the PR mentioned in the subject. When expanding >>> atomic_test_and_set we try the corresponding sync optabs and if none >>> exist, like for pre-SMP ARM architectures (-march=armv6 and earlier) the >>> code in optabs.c reverts to a single-threaded implementation that just >>> does a load and store. >>> >>> However, if the result of the operation is to be ignored the code in >>> builtins.c follows the convention that the target RTX is set to >>> const0_rtx to signify that the result is ignored and the expand >>> functions in optabs.c are supposed to check for that and act >>> appropriately. >>> >>> The code in the single-threaded codepath in expand_atomic_test_and_set >>> in optabs.c didn't perform this check and instead tried to emit a move >>> to a const0_rtx which ICEs further down the line. >>> >>> This patch fixes that by checking if the result is ignored, and if it is >>> omits the load. >>> I wouldn't dare to remove the load in normal atomic handling code due to >>> all the memory ordering subtleties, but this code path occurs only when >>> we have a trivially single-threaded bare-metal target and the compiler >>> assumes no races anyway and no dodgy context switches and tries to >>> implement this with a ldrb+strb, so I think removing the ldrb is valid. >>> >>> Bootstrapped on arm, aarch64 and x86 and tested as well. >>> >>> Ok for trunk? >>> This appears on 4.9 as well, I'll test it on that branch as well >>> >>> 2014-07-16 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>> >>> PR target/61713 >>> * gcc/optabs.c (expand_atomic_test_and_set): Do not try to emit >>> move to subtarget in serial version if result is ignored. > OK. > jeff Thanks Jeff, Is this ok for the 4.9 branch as well if testing comes back ok? (It applies cleanly there.) Kyrill >
On 08/04/14 10:54, Kyrill Tkachov wrote: > > On 25/07/14 23:05, Jeff Law wrote: >> On 07/23/14 02:53, Kyrill Tkachov wrote: >>> Darn, had forgotten to attach the patch... >>> >>> >>> On 16/07/14 12:30, Kyrill Tkachov wrote: >>>> Hi all, >>>> >>>> This fixes the PR mentioned in the subject. When expanding >>>> atomic_test_and_set we try the corresponding sync optabs and if none >>>> exist, like for pre-SMP ARM architectures (-march=armv6 and earlier) >>>> the >>>> code in optabs.c reverts to a single-threaded implementation that just >>>> does a load and store. >>>> >>>> However, if the result of the operation is to be ignored the code in >>>> builtins.c follows the convention that the target RTX is set to >>>> const0_rtx to signify that the result is ignored and the expand >>>> functions in optabs.c are supposed to check for that and act >>>> appropriately. >>>> >>>> The code in the single-threaded codepath in expand_atomic_test_and_set >>>> in optabs.c didn't perform this check and instead tried to emit a move >>>> to a const0_rtx which ICEs further down the line. >>>> >>>> This patch fixes that by checking if the result is ignored, and if >>>> it is >>>> omits the load. >>>> I wouldn't dare to remove the load in normal atomic handling code >>>> due to >>>> all the memory ordering subtleties, but this code path occurs only when >>>> we have a trivially single-threaded bare-metal target and the compiler >>>> assumes no races anyway and no dodgy context switches and tries to >>>> implement this with a ldrb+strb, so I think removing the ldrb is valid. >>>> >>>> Bootstrapped on arm, aarch64 and x86 and tested as well. >>>> >>>> Ok for trunk? >>>> This appears on 4.9 as well, I'll test it on that branch as well >>>> >>>> 2014-07-16 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>> >>>> PR target/61713 >>>> * gcc/optabs.c (expand_atomic_test_and_set): Do not try to emit >>>> move to subtarget in serial version if result is ignored. >> OK. >> jeff > > Thanks Jeff, > Is this ok for the 4.9 branch as well if testing comes back ok? (It > applies cleanly there.) Yes. jeff
Hi all, CC'ing release manager, Is this ok to backport to 4.9? Tested there with no problems. Kyrill On 04/08/14 17:54, Kyrill Tkachov wrote: > On 25/07/14 23:05, Jeff Law wrote: >> On 07/23/14 02:53, Kyrill Tkachov wrote: >>> Darn, had forgotten to attach the patch... >>> >>> >>> On 16/07/14 12:30, Kyrill Tkachov wrote: >>>> Hi all, >>>> >>>> This fixes the PR mentioned in the subject. When expanding >>>> atomic_test_and_set we try the corresponding sync optabs and if none >>>> exist, like for pre-SMP ARM architectures (-march=armv6 and earlier) the >>>> code in optabs.c reverts to a single-threaded implementation that just >>>> does a load and store. >>>> >>>> However, if the result of the operation is to be ignored the code in >>>> builtins.c follows the convention that the target RTX is set to >>>> const0_rtx to signify that the result is ignored and the expand >>>> functions in optabs.c are supposed to check for that and act >>>> appropriately. >>>> >>>> The code in the single-threaded codepath in expand_atomic_test_and_set >>>> in optabs.c didn't perform this check and instead tried to emit a move >>>> to a const0_rtx which ICEs further down the line. >>>> >>>> This patch fixes that by checking if the result is ignored, and if it is >>>> omits the load. >>>> I wouldn't dare to remove the load in normal atomic handling code due to >>>> all the memory ordering subtleties, but this code path occurs only when >>>> we have a trivially single-threaded bare-metal target and the compiler >>>> assumes no races anyway and no dodgy context switches and tries to >>>> implement this with a ldrb+strb, so I think removing the ldrb is valid. >>>> >>>> Bootstrapped on arm, aarch64 and x86 and tested as well. >>>> >>>> Ok for trunk? >>>> This appears on 4.9 as well, I'll test it on that branch as well >>>> >>>> 2014-07-16 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>> >>>> PR target/61713 >>>> * gcc/optabs.c (expand_atomic_test_and_set): Do not try to emit >>>> move to subtarget in serial version if result is ignored. >> OK. >> jeff > Thanks Jeff, > Is this ok for the 4.9 branch as well if testing comes back ok? (It > applies cleanly there.) > > Kyrill > > >
On 14/08/14 09:09, Kyrill Tkachov wrote: > Hi all, > > CC'ing release manager, > > Is this ok to backport to 4.9? Tested there with no problems. Ah, I see Jeff already ok'd it, sorry for the noise, must have missed then. Kyrill > Kyrill > > > On 04/08/14 17:54, Kyrill Tkachov wrote: >> On 25/07/14 23:05, Jeff Law wrote: >>> On 07/23/14 02:53, Kyrill Tkachov wrote: >>>> Darn, had forgotten to attach the patch... >>>> >>>> >>>> On 16/07/14 12:30, Kyrill Tkachov wrote: >>>>> Hi all, >>>>> >>>>> This fixes the PR mentioned in the subject. When expanding >>>>> atomic_test_and_set we try the corresponding sync optabs and if none >>>>> exist, like for pre-SMP ARM architectures (-march=armv6 and earlier) the >>>>> code in optabs.c reverts to a single-threaded implementation that just >>>>> does a load and store. >>>>> >>>>> However, if the result of the operation is to be ignored the code in >>>>> builtins.c follows the convention that the target RTX is set to >>>>> const0_rtx to signify that the result is ignored and the expand >>>>> functions in optabs.c are supposed to check for that and act >>>>> appropriately. >>>>> >>>>> The code in the single-threaded codepath in expand_atomic_test_and_set >>>>> in optabs.c didn't perform this check and instead tried to emit a move >>>>> to a const0_rtx which ICEs further down the line. >>>>> >>>>> This patch fixes that by checking if the result is ignored, and if it is >>>>> omits the load. >>>>> I wouldn't dare to remove the load in normal atomic handling code due to >>>>> all the memory ordering subtleties, but this code path occurs only when >>>>> we have a trivially single-threaded bare-metal target and the compiler >>>>> assumes no races anyway and no dodgy context switches and tries to >>>>> implement this with a ldrb+strb, so I think removing the ldrb is valid. >>>>> >>>>> Bootstrapped on arm, aarch64 and x86 and tested as well. >>>>> >>>>> Ok for trunk? >>>>> This appears on 4.9 as well, I'll test it on that branch as well >>>>> >>>>> 2014-07-16 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>>>> >>>>> PR target/61713 >>>>> * gcc/optabs.c (expand_atomic_test_and_set): Do not try to emit >>>>> move to subtarget in serial version if result is ignored. >>> OK. >>> jeff >> Thanks Jeff, >> Is this ok for the 4.9 branch as well if testing comes back ok? (It >> applies cleanly there.) >> >> Kyrill >> >> >> > >
commit 07d5e75e9b8816a2ce7c38d2a1a79e004fd778f3
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Wed Jul 9 14:13:09 2014 +0100
[ARM] PR target/61756 Fix atomic test and set if result is ignored
diff --git a/gcc/optabs.c b/gcc/optabs.c
index 7ee84c4..95147ae 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -7352,7 +7352,10 @@ expand_atomic_test_and_set (rtx target, rtx mem, enum memmodel model)
perform the operation. */
if (!ret)
{
- emit_move_insn (subtarget, mem);
+ /* If the result is ignored skip the move to target. */
+ if (subtarget != const0_rtx)
+ emit_move_insn (subtarget, mem);
+
emit_move_insn (mem, trueval);
ret = subtarget;
}
diff --git a/gcc/testsuite/gcc.dg/pr61756.c b/gcc/testsuite/gcc.dg/pr61756.c
new file mode 100644
index 0000000..c021290
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr61756.c
@@ -0,0 +1,15 @@
+/* PR target/61756 */
+
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-options "-O2 -march=armv5" { target arm*-*-* } } */
+
+#include <stdatomic.h>
+
+static volatile atomic_flag guard = ATOMIC_FLAG_INIT;
+
+void
+try_atomic_flag_test_and_set (void)
+{
+ atomic_flag_test_and_set (&guard);
+}