<p dir="ltr">(Sorry for top post. On phone.) </p>
<p dir="ltr">What I'm proposing is just the minimal change to get the thing building as designed.</p>
<p dir="ltr">I would agree that refactoring this would be a good idea, but that would be a logically separate change. Let's not conflate the two topics or gate the patch on refactoring. Can we start another thread about refactoring? I have some ideas about the subject (particularly on cleaning up the build system), but haven't acted on them yet.</p>
<div class="gmail_quote">On Dec 23, 2013 5:04 PM, C. Bergström <<a href="mailto:cbergstrom@pathscale.com">cbergstrom@pathscale.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 12/24/13 07:45 AM, Steven Noonan wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
On Mon, Dec 23, 2013 at 4:27 PM, "C. Bergström" <<a href="mailto:cbergstrom@pathscale.com" target="_blank">cbergstrom@pathscale.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 12/24/13 04:58 AM, Steven Noonan wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Mon, Dec 23, 2013 at 05:36:04PM +0000, Cownie, James H wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
For your Christmas hacking pleasure.<br>
<br>
This release aligns with Intel(r) Composer XE 2013 SP1 Product Update 2<br>
<br>
New features<br>
* The library can now be built with clang (though with some<br>
limitations since clang does not support 128 bit floats, so the library built with<br>
clang cannot be used with a different compiler that does have such support.)<br>
* Support for Vtune analysis of load imbalance<br>
* Code contribution from Steven Noonan to build the runtime for ARM*<br>
architecture processors<br>
</blockquote>
Cool. Thanks, Jim!<br>
<br>
It looks like there's still a minor correction that needs to happen. I<br>
didn't get an opportunity to test the merged changes until they went<br>
public, and there's actually a compile error in there now:<br>
<br>
../../src/z_Linux_asm.s: Assembler messages:<br>
../../src/z_Linux_asm.s:1590: Error: junk at end of line, first unrecognized character is `,'<br>
<br>
With a quick bit of Googling, I found this:<br>
<br>
<a href="http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=422971" target="_blank">http://bugs.debian.org/cgi-<u></u>bin/bugreport.cgi?bug=422971</a><br>
<br>
In that bug report's comments, Daniel Jacobowitz clarifies that '@' is<br>
an ARM comment marker, so using '%' is the preferred annotation. I've<br>
compile and run-tested this on both x86_64 and ARM, and both work at<br>
this point:<br>
<br>
diff --git a/runtime/src/z_Linux_asm.s b/runtime/src/z_Linux_asm.s<br>
index 1f1ba1b..bba3b14 100644<br>
--- a/runtime/src/z_Linux_asm.s<br>
+++ b/runtime/src/z_Linux_asm.s<br>
@@ -1587,5 +1587,5 @@ __kmp_unnamed_critical_addr:<br>
<br>
<br>
#if defined(__linux__)<br>
-.section .note.GNU-stack,"",@progbits<br>
+.section .note.GNU-stack,"",%progbits<br>
#endif<br>
</blockquote>
That change is almost certainly incorrect (we've had to fix that same problem in libunwind and openblas before)<br>
</blockquote>
With a bit more searching around, I think the correct answer is actually this:<br>
<br>
#if defined (__linux__)<br>
#if KMP_ARCH_ARM<br>
.section .note.GNU-stack,"",%progbits<br>
#else<br>
.section .note.GNU-stack,"",@progbits<br>
#endif<br>
#endif<br>
<br>
Based on binutils documentation:<br>
<a href="https://sourceware.org/binutils/docs-2.18/as/Section.html" target="_blank">https://sourceware.org/<u></u>binutils/docs-2.18/as/Section.<u></u>html</a><br>
<br>
"Note on targets where the @ character is the start of a comment (eg ARM) then<br>
another character is used instead. For example the ARM port uses the %<br>
character."<br>
<br>
So on x86, @ is correct, while on ARM % is correct.<br>
<br>
Better patch:<br>
<br>
diff --git a/runtime/src/z_Linux_asm.s b/runtime/src/z_Linux_asm.s<br>
index 1f1ba1b..0a96d38 100644<br>
--- a/runtime/src/z_Linux_asm.s<br>
+++ b/runtime/src/z_Linux_asm.s<br>
@@ -1587,5 +1587,9 @@ __kmp_unnamed_critical_addr:<br>
<br>
<br>
#if defined(__linux__)<br>
+#if KMP_ARCH_ARM<br>
+.section .note.GNU-stack,"",%progbits<br>
+#else<br>
.section .note.GNU-stack,"",@progbits<br>
#endif<br>
+#endif<br>
</blockquote>
2 nits while we're here<br>
<br>
#1 (typically?) .S indicates the asm file needs to be pre-processed and .s doesn't. I'd suggest to rename the file<br>
<br>
#2 It may arguably be better long term design to have something like<br>
runtime/src/arm/z_Linux_asm.s<br>
runtime/src/amd64/z_Linux_asm.<u></u>s<br>
<br>
this may add some duplication, but avoids yucky macro hell since that file is likely to be a bane of portability. I wonder if Hal/Jeff (who worked on the BGQ port) have any comments<br>
<br>
<br>
</blockquote></div>