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

[Alexey Bataev via cfe-commits]

ABataev marked an inline comment as done.
ABataev added a comment.

Hi Chandler, thanks for the review.

In http://reviews.llvm.org/D13802#272053, @chandlerc wrote:

> 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 created a separate patch for libomp, that fixes this issue.

> 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.


Actually, these tests are written so, that they repeats about 1000 times to be sure that they are correct. But some of them might be sensible to system load.


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

================
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__':
----------------
chandlerc wrote:
> 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.
Ok, reverted it


http://reviews.llvm.org/D13802