[PATCH] D13802: [OPENMP] Make -fopenmp to turn on OpenMP support by default.

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Some minor issues below. Also, Hans and Tom should have a glance at the release bits of this, I don't know any of that stuff.

Also, there are still some bugs with check-libomp. The immediate one I noticed was that when i run it in a clean build with enough parallelism, it fails to find the just-built clang binary. I suspect that the 'check-libomp' CMake target is missing dependencies on all of the binaries used when running the lit tests because it copied the libc++ CMake rules for it. While the libc++ CMake rules are *mostly* a good proxy, the *testing* strategy for libc++ is notably different here. In fact, libomp's testing is (IMO) quite a bit *better* here, and uncovers a nasty missing dependency.

The libc++ test suite tests libc++ with the *host* compiler, not the just-built compiler. This has some advantages and disadvantages, but for a library with close coupling to the compiler (such as compiler-rt or libomp) it is inappropriate. Your libomp tests very correctly use the just-built Clang, but the CMake needs to model this now. You can see in projects/compiler-rt/test/CMakeLists.txt lines 19-33 how compiler-rt sets up variables with these tools (clang, clang-headers, etc) which end up used on line 99 of projects/compiler-rt/test/asan/CMakeLists.txt for example.

I don't know what all tools you need (other than clang and probably clang-headers), probably not as many as the sanitizers do, so I'll let you put together a correct patch here. I've hacked around it locally to test further.

I've also had one test fail, and then start passing for me on Linux (after fixing the above). I haven't had it fail again, but I don't have a good way of running tests repeatedly (see below, llvm-lit doesn't yet work). It might be good to give the test suite a good 10 or 50 runs and see if anything starts failing. I'll do that overnight and report back if so.


================
Comment at: configure:5953
@@ -5952,3 +5952,3 @@
 else
-  withval="libgomp"
+  withval="libomp"
 fi
----------------
You should be modifying the configure.ac file and regenerating this?

================
Comment at: utils/llvm-lit/llvm-lit.in:42-46
@@ -41,2 +41,7 @@
 
+openmp_obj_root = os.path.join(llvm_obj_root, 'projects', 'openmp')
+if os.path.exists(openmp_obj_root):
+    builtin_parameters['openmp_site_basedir'] = \
+            os.path.join(openmp_obj_root, 'runtime', 'test')
+
 if __name__=='__main__':
----------------
This change is independent of enabling anything; can you break it out?

Also, manually patching this bit in order to test things didn't actually allow me to use llvm-lit with libomp, so I don't think this is quite working as intended yet.


http://reviews.llvm.org/D13802