[PATCH] D10656: LLVM OpenMP CMake Overhaul

[Jonathan Peyton]

jlpeyton added a comment.

Addressing Ed Maste's comments directly in Phabricator (they were already addressed silently in the code)

> First and foremost, thank you again for working on this.

> 

> I want to call out one thing that was very helpful in case it went unnoticed. Your replies of things that you're planning to do in subsequent iterations and your replies giving context as to *why* things are the way they are. Totally useful.

> 

> Also, I appreciate the added comments archiving context for the next reader. I don't want you to feel like that was wasted time, it helps tremendously.

> 

> At this point, I think you should commit this after addressing the remaining (minor) comments from Ed Maste and some of the simple things below. This is *significantly* better than the starting point, and I'm interested in letting you get to the next iteration.

> 

> Once it lands, I'll also test out using it, and look to see if I spot any actual functional issues.


Chandler,

Thanks for reviewing this patch.  I know it wasn't the ideal in terms of size, but needed to be done.  I appreciate your feedback and direction.

I apologize but didn't see the recommendation to change the indention to two space indention.  So I *QUICKLY* committed a follow up patch which addressed your comments.  I changed the indention (which I understand makes an awful patch) and moved the comment about enabling MASM.  Regarding the prof and stubs library, the prof library is something I need to remove, and I'm not opposed to changing the stubs library name.  I'm not sure anyone has actually built the stubs library except for me... :) .  And in case anyone was wondering what the stubs library is for, it is a minimal, serial implementation of the OpenMP runtime.  For example, omp_get_thread_num() will always return 0 in the stubs library.  When someone doesn't want to compile there OpenMP code with -fopenmp, but needs the omp_* API, they can link to the stubs library.

Also thanks to Jack Howarth, Ed Maste, Chris Bergstrom, Jim Cownie, for leaving feeback, and testing the patch out.


================
Comment at: runtime/cmake/LibompMicroTests.cmake:19
@@ +18,3 @@
+#    - Available for all Unix,Mac,Windows builds.  Not available on Intel(R) MIC Architecture builds.
+# (2) test-relo
+#    - Tests dynamic libraries for position-dependent code (can not have any position dependent code)
----------------
I've changed all these to just Unix so we don't have to enumerate all the Unix flavors.

================
Comment at: runtime/cmake/LibompMicroTests.cmake:34
@@ +33,3 @@
+#    - Available for Intel(R) MIC Architecture and i386 builds. Not available otherwise.
+# (5) test-deps
+#    - Tests newly created libomp for library dependencies
----------------
I've added to the comment that i386 architecture is supported as well.

================
Comment at: runtime/cmake/LibompMicroTests.cmake:39
@@ +38,3 @@
+#    - Available for Unix,Mac,Windows, Intel(R) MIC Architecture dynamic builds and Windows
+#      static builds. Not available otherwise.
+
----------------
I've removed the libunwind library from FreeBSD's list.


Repository:
  rL LLVM

http://reviews.llvm.org/D10656