Identified a while before the release, there is an issue with 64-bit atomics on ARM that was making Clang mis-compile a lot of code, including Clang itself.
http://llvm.org/bugs/show_bug.cgi?id=15429
Attached is a patch proposed by Benjamin with the corrections to the test.
I’m not an expert on how Clang lowers C11 atomics, but the resulting IR seems correct, and after self-hosting Clang with this patch, it managed to pass all test-suite (as before), which is a good indication that the patch does fix the problem.
Please review!
cheers,
–renato
atomic_arm.patch (6.78 KB)
Hi Rafael,
As you mentioned in the bug, we should only apply this change when hard-float is set, which it is by default on armv7a, I presume.
From that part of the code, I can infer that by the time “MaxAtomicPromoteWidth = 64;”, the variable SoftFloat is not properly set, so a simple “if (!SoftFloat)” won’t cut in there. It seems SoftFloat is being set on HandleTargetFeatures() which is a virtual method, probably called indirectly. Do you have a better place to set MaxAtomicInlineWidth = 64?
cheers,
–renato
I have asked on #gcc what gcc does. I have posted a detailed
description in the bug, but the summary is that there is some
cooperation with the kernel going on that should make it safe to set
MaxAtomicInlineWidth to 64 when targeting linux armv6 or newer. Hard
float using it then just becomes a consequence of it implying armv7.
I know think we need something like the attached patch (with tests and
comments added).
Cheers,
Rafael
t.patch (1.11 KB)
I was afraid you were going to suggest that... 
If that's the consensus, than I'll merge our patches, test and commit.
cheers,
--renato
I have asked on #gcc what gcc does. I have posted a detailed
description in the bug, but the summary is that there is some
cooperation with the kernel going on that should make it safe to set
MaxAtomicInlineWidth to 64 when targeting linux armv6 or newer. Hard
float using it then just becomes a consequence of it implying armv7.
I know think we need something like the attached patch (with tests and
comments added).
OK. The attached patch has comments and a test.
Renato, what do you think? I am currently building it on a chromebook
to make sure if fixes the bootstrap, but that will take some time.
Cheers,
Rafael
t.patch (2.06 KB)
There's also the test that fails, which I have in my patch. It should fail
to you too.
Maybe we should also test armv5 on your test, to make sure the behaviour
remains different (and documented).
I'm applying your patch to my tree now and will let you know.
cheers,
--renato
There's also the test that fails, which I have in my patch. It should fail
to you too.
No, that test is using a bsd triple, so my patch doesn't change it. I
don't know how the various BSDs implement this.
Maybe we should also test armv5 on your test, to make sure the behaviour
remains different (and documented).
The current behavior (with and without the patch) is to error since we
know that we need to produce a function call, but we don't have code
to do it.
I'm applying your patch to my tree now and will let you know.
Thanks,
Rafael
Makes sense. Please, commit.
--renato
Shouldn’t you also support Thumb? And what about after v7 (e.g. v8) which AFAIK also supports these?
Shouldn't you also support Thumb?
Thanks for checking. Looks like for thumb we should use atomic
instructions only for v7 and newer. At least that is what gcc does.
And what about after v7 (e.g. v8) which
AFAIK also supports these?
There is no v8 support at the moment in clang, but yes, the patch
should have been more future proof. I will send an update.
Cheers,
Rafael