[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

Tue Nov 12 13:34:43 PST 2019

jdoerfert marked 3 inline comments as done.
jdoerfert added inline comments.

================
Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG
----------------
JonChesterfield wrote:
> Meinersbur wrote:
> > JonChesterfield wrote:
> > > jdoerfert wrote:
> > > > Meinersbur wrote:
> > > > > jdoerfert wrote:
> > > > > > JonChesterfield wrote:
> > > > > > > Sharing constants between the compiler and the runtime is an interesting subproblem. I think the cleanest solution is the one used by libc, where the compiler generates header files containing the constants which the runtime library includes.
> > > > > > I'd prefer not to tackle this right now but get this done first and revisit the issue later. OK?
> > > > > I don't think this is a good solution. It means that libomp cannot built built anymore without also building clang. Moreover, the values cannot be changed anyway since it would break the ABI.
> > > > > 
> > > > > I'd go the other route: The libomp defines what it's ABI is, the compiler generates code for it. 
> > > > This patch doesn't change what we do, just where. The numbers are hard coded in clang now. Let's start a discussion on the list and if we come up with a different scheme we do it after this landed.
> > > Revisit later sounds good.
> > > 
> > > @Meinersbur Do you know of an example of a non-llvm compiler using this libomp?
> > > 
> > > The usual order is build a compiler, then use it to build the runtime libraries, then the whole package can build other stuff. Provided the compiler doesn't need any of the runtime libraries (compiler-rt, maths libraries, libomp etc) itself the system bootstraps cleanly. Especially important when cross compiling and I suspect the gpu targets in openmp have similarly strict requirements on the first compiler.
> > > 
> > > Closely related to that, I tend to assume that the runtime libraries can be rewritten to best serve their only client - the associated compiler - so if libomp is used by out of tree compilers I'd like to know who we are at risk of breaking.
> > > Do you know of an example of a non-llvm compiler using this libomp?
> > 
> > [[ https://github.com/llvm-project/llvm-project/blob/master/openmp/runtime/src/kmp_gsupport.cpp | gcc  ]] (using libomp's gomp compatibility layer), [[ https://www.openmprtl.org/ | icc  ]] (as libomp was initially donated by Intel).
> > 
> > I don't understand why it even matters if there are other compilers using libomp. Every LLVM runtime library can be built stand-alone. 
> > With constant values being determined during compiler bootstrapping, programs built on one computer would be potentially ABI-incompatible with a runtime library on another. Think about updating your compiler-rt/libomp/libc++ on you computer causing all existing binaries on the system to crash because constants changed in the updated compiler's bootstrapping process.
> > 
> > The only use case I know that does this is are operating system's syscall tables. Linux's reference is [[ https://github.com/torvalds/linux/blob/master/arch/sh/include/uapi/asm/unistd_64.h | unistd.h ]] which is platform-specific and Windows generates the table during its [[ https://j00ru.vexillium.org/syscalls/nt/64/ | build process ]]. Therefore on Windows, system calls can only be done through ntdll. Even on Linux one should use the system's libc instead of directly invoking a system call.
> Thanks. GCC and ICC would presumably be happier with the magic numbers stored with openmp then (though with the move to a monorepo that's a little less persuasive).
> 
> When constants that affect the ABI change, the result won't work with existing software regardless of whether the compiler or the library contains the change. Either the new compiler builds things that don't work with the old library, or the new library doesn't work with things built by the old compiler. The two have to agree on the ABI.
> 
> At present, openmp does the moral equivalent of #include OpenMPKinds.def from clang. Moving the constants to libomp means clang will do the equivalent of #include OpenMPKinds.def from openmp. Breaking that dependency means making a new subproject that just holds/generates the constants, that both depend on, which seems more hassle than it's worth. 
> 
> I'd like to generate this header as part of the clang build (though ultimately don't care that much if it's generated as part of the openmp build) because it's going to become increasingly challenging to read as non-nvptx architectures are introduced. Likewise it would be useful to generate the interface.h for deviceRTL (or equivalently a set of unit tests checking the function types) from the same source to ensure it matches and that's not economically feasible within the C preprocessor.
I am unsure how this conversation evolved and what you want me to do now.

I repeat what I said before:

This does neither change the constants, our usage of them, nor the fact that we have them defined in multiple places, just one of the places (now llvm, before clang) changed.

================
Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:179
+        Builder.CreateCall(FnDecl, Ident, "omp_global_thread_num");
+    if (Instruction *IdentI = dyn_cast<Instruction>(Ident))
+      Call->moveAfter(IdentI);
----------------
ABataev wrote:
> `auto *`
Sure.

================
Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+                   getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  Value *Result = Builder.CreateCall(
----------------
ABataev wrote:
> Maybe add an assert when the cancellation version is requested but the cancellation block is not set? Instead of the generating simple version of barrier.
The interface doesn't work that way as we do not know here if the cancellation was requested except if the block was set. That is basically the flag (and I expect it to continue to be that way).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69785/new/

https://reviews.llvm.org/D69785