llvm dev - May 2013 - [LLVMdev] [PATCH] 3.3 Release fix on ARM - atomics

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130513/96b2ef27/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: atomic_arm.patch
Type: application/octet-stream
Size: 6943 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130513/96b2ef27/attachment.obj>

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


On 13 May 2013 13:56, Renato Golin <renato.golin at linaro.org> wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130513/c2b8fbc4/attachment.html>

On 13 May 2013 10:59, Renato Golin <renato.golin at linaro.org>
wrote:
> 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?
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.patch
Type: application/octet-stream
Size: 1135 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130513/d36a4baf/attachment.obj>