[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

Wed May 27 20:44:11 PDT 2020

jhuber6 marked an inline comment as done.
jhuber6 added inline comments.

================
Comment at: openmp/runtime/test/tasking/kmp_taskloop.c:100
         th_counter[i] = 0;
-    #pragma omp parallel num_threads(N)
+    #pragma omp parallel // num_threads(N)
     {
----------------
jdoerfert wrote:
> jhuber6 wrote:
> > jdoerfert wrote:
> > > jhuber6 wrote:
> > > > jdoerfert wrote:
> > > > > jhuber6 wrote:
> > > > > > AndreyChurbanov wrote:
> > > > > > > jhuber6 wrote:
> > > > > > > > jhuber6 wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > jhuber6 wrote:
> > > > > > > > > > > I am not entirely sure why, but commenting this out causes the problem to go away. I tried adding proper names to the forward-declared functions but since clang already knew I had something called ident_t, I couldn't declare a new struct with the same name.
> > > > > > > > > > This is not good. The difference should only be that the `kmpc_fork_call` has a different argument, right? Does the segfault happen at compile or runtime?
> > > > > > > > > > 
> > > > > > > > > > You can just use the ident_t clang created, right? Did you print the function names requested by clang as we discussed?
> > > > > > > > > I added an assertion and debug statements. If I try to declare a struct named "Ident_t" I get the following error message in the seg-fault. I think the seg-fault is compile-time.
> > > > > > > > > 
> > > > > > > > > Found OpenMP runtime function __kmpc_global_thread_num with type i32 (%struct.ident_t.0*). Expected type is i32 (%struct.ident_t*)
> > > > > > > > > clang: /home/jhuber/Documents/llvm-project/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:124: static llvm::Function* llvm::OpenMPIRBuilder::getOrCreateRuntimeFunction(llvm::Module&, llvm::omp::RuntimeFunction): Assertion `FnTy == Fn->getFunctionType() && "Found OpenMP runtime function has mismatched types"' failed.
> > > > > > > > I'm not sure if there's a way around this without changing the getOrCreateRuntimeFunction method to return a FunctionCallee and removing the assertion. Clang doesn't know about the ident_t struct when it's compiling the file, but when its doing the codegen it sees two structs with the same name and creates a new name. So when it gets the types it says that ident_t and ident_t.0 don't match. As you said the old version got around this by adding a bitcasting instruction so it knew how to turn it into an ident_t pointer.
> > > > > > > Note that this change breaks the test on any system with more that 4 procs.  Because array th_counter[4] is indexed by thread number which can easily be greater than 3 if number of threads is not limited.
> > > > > > The problem was that the num_threads clause required an implicit call to kmpc_global_thread_num so it could be passed to kmpc_push_num_threads. The types of the implicit function and the forward declaration then wouldn't match up. I added another forward declaration to explicitly call kmpc_push_num_threads. Is this a sufficient solution?
> > > > > We need this to work with num_threads(8). 
> > > > > 
> > > > > > Clang doesn't know about the ident_t struct when it's compiling the file, but when its doing the codegen it sees two structs with the same name and creates a new name.
> > > > > 
> > > > > Where are the two structs coming from? We should have one. If clang introduces one it needs to use the one from OMPKindes.def instead. Is that a fix?
> > > > The first struct is the one that I'm assuming comes from the OpenMP CodeGen that places the Ident_t struct in the IR file. if I declare a struct also named ident_t in the C source file it most likely will see that there's two structs with the same name and call the second one "ident_t.0" internally. The other ident_t struct is only known once clang generates the LLVM IR so I can't just use "ident_t" nor can I declare a struct with the same name.
> > > 1) Either Clang needs to use the `llvm::omp::types::Ident` *or* Clang needs to define `llvm::omp::types::Ident` and we do not do it via  `__OMP_STRUCT_TYPE(Ident, ident_t, Int32, Int32, Int32, Int32, Int8Ptr)`. I would prefer the first solution. 
> > > 
> > > 2) `OMPConstants.cpp` does pick up an existing struct type with the same name if present. That is, probably not what we want because it clashes with user types.
> > > 
> > > 3) We can still return a `FunctionCallee` with the Function* and the expected type (as defined by OMPKinds.def) to mimic the old behavior for now.
> > > Either Clang needs to use the `llvm::omp::types::Ident` *or* Clang needs to define `llvm::omp::types::Ident` and we do not do it via `__OMP_STRUCT_TYPE(Ident, ident_t, Int32, Int32, Int32, Int32, Int8Ptr)`. I would prefer the first solution.
> > I'm probably not understanding something correctly here. There's already an ident_t type declared in `CGOpenMPRuntime:1065` which it uses for generating the types for the runtime functions, but this isn't used until code generation so it can't be used while compiling the file. If we declare it up front then wouldn't that make ident_t a reserved keyword?
> > 
> > > We can still return a `FunctionCallee` with the `Function*` and the expected type (as defined by OMPKinds.def) to mimic the old behavior for now.
> > A FunctionCallee can be generated from a Function * but not vice-versa, right? This would require changing the code in OpenMPIRBuilder.
> In CGOpenMPRuntime.cpp there is:
> `IdentTy = CGM.getTypes().ConvertRecordDeclType(RD);`
> what happens if you replace that with:
> `IdentTy = llvm::omp::types::Ident;`
> ? 
> 
> ---
> 
> FunctionCallee is just a Function* and the "expected type". You can go from FunctionCallee to Function by just picking the proper member. You'd need to change the Builder in two ways: 1) return FunctionCallee(Fn, ExpectedType); 2) where it is used extract the Function * from the FunctionCallee. You might want to use a helper if you go this route.
It causes all the OpenMP tests to fail, not sure why exactly. I'll look into it more tomorrow.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222