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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Oh, sorry. You merged all of my patch, right? It was not ready and even now it 
is unclear if the representation change in my patch is good or not. I was 
trying to suggest you only take the InternalOMPIRBuilder stuff so you can avoid 
the static function, not all of my patch. Sorry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

In D80222#2065141 , @jdoerfert wrote:

> Initialize the internal OpenMPIRbuilder (see my patch).


Isn't it initialized on line 1063 in `CGOpenMPRuntime.cpp`? There weren't any 
conflicts listed there when I merged your patch so it should be the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Initialize the internal OpenMPIRbuilder (see my patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

In D80222#2061810 , @jdoerfert wrote:

> LGTM. Thanks for taking this one, it was more complex than I thought but it 
> is a really nice step in the right direction. I'll commit it for you soon if 
> you don't have access yet. Feel free to get access though.


Thanks, I don't have access yet. I'll get on that soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks for taking this one, it was more complex than I thought but it is 
a really nice step in the right direction. I'll commit it for you soon if you 
don't have access yet. Feel free to get access though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I figured out what the problem is and we can relatively easily fix it *once* we 
always have an OpenMPIRBuilder available in Clangs CG. As noted in D80735 
, we should remove `IdentQTy` as it is used to 
create a new ident_t in some situations. The entire 
`getOrCreateDefaultLocation` in clangs CG, should be replaced by 
`OpenMPIRBuilder::getOrCreateIdent`. We do this later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

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

Yes, this passed all the tests on my machine at least.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:124
+  return {FnTy, Fn};
+}
   }

jdoerfert wrote:
> Are you sure we need to do the cast here? I thought clang will create it when 
> it creates calls based of FunctionCallee objects. That was, to my 
> understanding, the idea of the FunctionCallee thing. We might be able to just 
> return {FnTy, Fn} here. If not we could always cast and return, as you noted 
> a function *. Internally, only the expected type makes any sense, everything 
> else will not be handled gracefully.
I tried it without and the same problem occured. The old method used 
`CGM.createRuntimeFunction` which internally has a cast like this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

This passes all the tests? I think we should go with it and investigate the 
cast thing later.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:124
+  return {FnTy, Fn};
+}
   }

Are you sure we need to do the cast here? I thought clang will create it when 
it creates calls based of FunctionCallee objects. That was, to my 
understanding, the idea of the FunctionCallee thing. We might be able to just 
return {FnTy, Fn} here. If not we could always cast and return, as you noted a 
function *. Internally, only the expected type makes any sense, everything else 
will not be handled gracefully.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:131
+  FunctionCallee RTLFn = getOrCreateRuntimeFunction(M, FnID);
+  auto *Fn = dyn_cast(RTLFn.getCallee());
   assert(Fn && "Failed to create OpenMP runtime function");

here we probably want to cast the result if we don't do it above. Just do the 
`getBitCast`, it should be a no-op if there is nothing to cast.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

In D80222#2060420 , @jdoerfert wrote:

> Take a look at D80735 , it works fine for me 
> locally. Is that what you did? What problems do you observe now?


That's pretty much what I did. If I kept in the old Ident declaration, but 
didn't use it and returned `llvm::omp::types::IdentPtr` from that function it 
would work. But the compiler would segfault if I removed it. It still failed 
the two problematic tests. I have a patch that changes the function to return a 
FunctionCallee and does a bitcast on the function if the types don't match. 
That passed the tests but it would be beneficial if we could get it to work 
without needing to change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Take a look at D80735 , it works fine for me 
locally. Is that what you did? What problems do you observe now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

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)
 {

jhuber6 wrote:
> 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, 
> 

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

[Joseph Huber via Phabricator via cfe-commits]

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 

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert 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)
 {

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 

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

[Joseph Huber via Phabricator via cfe-commits]

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:
> > > > 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, 

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert 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)
 {

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert 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)
 {

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

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)
 {

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Andrey Churbanov via Phabricator via cfe-commits]

AndreyChurbanov 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)
 {

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

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)
 {

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:250
+__OMP_SIZE_TYPE(SizeTy)
+#undef __OMP_SIZE_TYPE
+

jdoerfert wrote:
> Why the indirection via `__OMP_SIZE_TYPE`? Wouldn't `OMP_TYPE(SizeTy, 
> M.getDataLayout().getIntPtrType(Ctx))` suffice?
I think I did it just to make it more explicit while it was temporary, but now 
that we're defining it here there's no point, I'll change it.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:124
+assert(FnTy == Fn->getFunctionType() &&
+   "Found OpenMP runtime function has mismatched types");
   }

jdoerfert wrote:
> I now think we either put the stuff in `#ifndef NDEBUG` or move the `FnTy =` 
> into the first switch that does the lookup.
Yeah, I was wondering about the cost of the extra lookup. But we could 
definitely just get the type when we look up the function and just do a 
comparison in this branch.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:250
+__OMP_SIZE_TYPE(SizeTy)
+#undef __OMP_SIZE_TYPE
+

Why the indirection via `__OMP_SIZE_TYPE`? Wouldn't `OMP_TYPE(SizeTy, 
M.getDataLayout().getIntPtrType(Ctx))` suffice?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:124
+assert(FnTy == Fn->getFunctionType() &&
+   "Found OpenMP runtime function has mismatched types");
   }

I now think we either put the stuff in `#ifndef NDEBUG` or move the `FnTy =` 
into the first switch that does the lookup.



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)
 {

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 marked 2 inline comments as done.
jhuber6 added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:245
+__OMP_PTR_TYPE(Int8PtrPtr, Int8Ptr)
+__OMP_PTR_TYPE(Int8PtrPtrPtr, Int8PtrPtr)
+

I added these types as @fghanim suggested.



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)
 {

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D80222#2055149 , @jhuber6 wrote:

> This passes all checks except the libomp ones. Those still seg-fault, I'm not 
> entirely sure why. Cuda failure is again because I don't have it configured 
> on my machine.
>
> Failing Tests (3):
>
>   Clang :: Driver/cuda-simple.cu
>   libomp :: tasking/kmp_taskloop.c
>   libomp :: worksharing/for/kmp_doacross_check.c


I believe libomp tests are not related to this but you should double check. run 
them locally without this patch maybe


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:244
 
-// TODO: Replace this with the real size_t type
-#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, Type::getInt64Ty(Ctx))
+#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, 
M.getDataLayout().getIntPtrType(Ctx))
 __OMP_SIZE_TYPE(SizeTy)

jhuber6 wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > jhuber6 wrote:
> > > > I'm just directly getting the SizeTy from the Module, I'm not sure if 
> > > > this is a permanent solution.
> > > I actually think this is fine. Less burden on the frontends also. 
> > > @fghanim looks good?
> > Agreed. But to avoid conflict with patch D79675, let this change go in a 
> > separate patch after that. This way, we can also do some clean ups related 
> > to this, and add `Int8PtrPtr`, `Int8PtrPtrPtr`, and any other typing 
> > issues, without delaying patch D79675 any further - I am done with that 
> > patch, and made my final changes a couple of days ago.
> > 
> > Nit: Also, I saw below that some formatting was done. Wouldn't it be better 
> > if formatting/comments related to changes in D79739, are handled as part of 
> > that patch?
> Doesn't this patch already support Int8PtrPtrPtr? It's the same thing as a 
> VoidPtrPtrPtr which I have defined above here.
You are correct. But think of it in the same way we have voidptr and int8ptr, 
even though they are the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:244
 
-// TODO: Replace this with the real size_t type
-#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, Type::getInt64Ty(Ctx))
+#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, 
M.getDataLayout().getIntPtrType(Ctx))
 __OMP_SIZE_TYPE(SizeTy)

fghanim wrote:
> jdoerfert wrote:
> > jhuber6 wrote:
> > > I'm just directly getting the SizeTy from the Module, I'm not sure if 
> > > this is a permanent solution.
> > I actually think this is fine. Less burden on the frontends also. @fghanim 
> > looks good?
> Agreed. But to avoid conflict with patch D79675, let this change go in a 
> separate patch after that. This way, we can also do some clean ups related to 
> this, and add `Int8PtrPtr`, `Int8PtrPtrPtr`, and any other typing issues, 
> without delaying patch D79675 any further - I am done with that patch, and 
> made my final changes a couple of days ago.
> 
> Nit: Also, I saw below that some formatting was done. Wouldn't it be better 
> if formatting/comments related to changes in D79739, are handled as part of 
> that patch?
Doesn't this patch already support Int8PtrPtrPtr? It's the same thing as a 
VoidPtrPtrPtr which I have defined above here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:244
 
-// TODO: Replace this with the real size_t type
-#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, Type::getInt64Ty(Ctx))
+#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, 
M.getDataLayout().getIntPtrType(Ctx))
 __OMP_SIZE_TYPE(SizeTy)

jdoerfert wrote:
> jhuber6 wrote:
> > I'm just directly getting the SizeTy from the Module, I'm not sure if this 
> > is a permanent solution.
> I actually think this is fine. Less burden on the frontends also. @fghanim 
> looks good?
Agreed. But to avoid conflict with patch D79675, let this change go in a 
separate patch after that. This way, we can also do some clean ups related to 
this, and add `Int8PtrPtr`, `Int8PtrPtrPtr`, and any other typing issues, 
without delaying patch D79675 any further - I am done with that patch, and made 
my final changes a couple of days ago.

Nit: Also, I saw below that some formatting was done. Wouldn't it be better if 
formatting/comments related to changes in D79739, are handled as part of that 
patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a subscriber: fghanim.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:244
 
-// TODO: Replace this with the real size_t type
-#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, Type::getInt64Ty(Ctx))
+#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, 
M.getDataLayout().getIntPtrType(Ctx))
 __OMP_SIZE_TYPE(SizeTy)

jhuber6 wrote:
> I'm just directly getting the SizeTy from the Module, I'm not sure if this is 
> a permanent solution.
I actually think this is fine. Less burden on the frontends also. @fghanim 
looks good?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 marked 2 inline comments as done.
jhuber6 added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:203
+  static Function *getOrCreateRuntimeFunction(Module ,
+  omp::RuntimeFunction FnID);
 

jdoerfert wrote:
> jhuber6 wrote:
> > jdoerfert wrote:
> > > Nit: M is the commonly used name for a module ;)
> > The OpenMPIRBuilder struct has a Module named M so I wasn't sure if I 
> > should be shadowing it.
> Go with `M`, it's static :)
Forgot to write the file when I changed it here, not work updating the diff for 
a single character. I'll put it in the next one.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:244
 
-// TODO: Replace this with the real size_t type
-#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, Type::getInt64Ty(Ctx))
+#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, 
M.getDataLayout().getIntPtrType(Ctx))
 __OMP_SIZE_TYPE(SizeTy)

I'm just directly getting the SizeTy from the Module, I'm not sure if this is a 
permanent solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

> libomp used to pass until I changed it to us getOrCreateRuntimeFunction I 
> think.

Those tests are flaky, I think. Just run them a few times. I don't expect this 
to influence them at all.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:203
+  static Function *getOrCreateRuntimeFunction(Module ,
+  omp::RuntimeFunction FnID);
 

jhuber6 wrote:
> jdoerfert wrote:
> > Nit: M is the commonly used name for a module ;)
> The OpenMPIRBuilder struct has a Module named M so I wasn't sure if I should 
> be shadowing it.
Go with `M`, it's static :)



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:483
+__OMP_RTL(__kmpc_push_num_teams, false, /* Void? */ Int32, IdentPtr, Int32, 
+  Int32, Int32)
 

jhuber6 wrote:
> For this one there's conflicting information on the return value. Clang 
> expects this in its checks
> // CK1: {{%.+}} = call i32 @__kmpc_push_num_teams({{.+}}, {{.+}}, i32 
> [[TE_VAL]], i32 [[TH_VAL]])
> 
> But when I look at the runtime I find this 
> void __kmpc_push_num_teams(ident_t *loc, kmp_int32 global_tid,
>kmp_int32 num_teams, kmp_int32 num_threads) {
> 
Make it void here please, will need to update the tests:
`sed -i -e 's:i32 @__kmpc_push_num_teams:void @__kmpc_push_num_teams:' 
clang/test/OpenMP/*.*`



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:103
+  }
+} else if (Fn->getName() == "__kmpc_fork_teams") {
+  if (!Fn->hasMetadata(LLVMContext::MD_callback)) {

jhuber6 wrote:
> jdoerfert wrote:
> > Please use `FnID` for the comparison.
> Don't know why I didn't think of doing that, will do.
And if the callback is the same for both, merge the conditionals.


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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:483
+__OMP_RTL(__kmpc_push_num_teams, false, /* Void? */ Int32, IdentPtr, Int32, 
+  Int32, Int32)
 

For this one there's conflicting information on the return value. Clang expects 
this in its checks
// CK1: {{%.+}} = call i32 @__kmpc_push_num_teams({{.+}}, {{.+}}, i32 
[[TE_VAL]], i32 [[TH_VAL]])

But when I look at the runtime I find this 
void __kmpc_push_num_teams(ident_t *loc, kmp_int32 global_tid,
   kmp_int32 num_teams, kmp_int32 num_threads) {



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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 marked 3 inline comments as done.
jhuber6 added a comment.

Here's the tests it fails, there might be a few that are wrong for reasons 
beyond the size_t stuff but it's hard to tell until that issue is resolved. The 
cuda test is just because I have CUDA set up incorrectly on my machine, libomp 
used to pass until I changed it to us getOrCreateRuntimeFunction I think.

  Clang :: Driver/cuda-simple.cu
  Clang :: OpenMP/barrier_codegen.cpp
  Clang :: OpenMP/distribute_parallel_for_reduction_codegen.cpp
  Clang :: OpenMP/distribute_parallel_for_reduction_task_codegen.cpp
  Clang :: OpenMP/for_reduction_task_codegen.cpp
  Clang :: OpenMP/nvptx_target_parallel_reduction_codegen.cpp
  Clang :: OpenMP/nvptx_teams_reduction_codegen.cpp
  Clang :: OpenMP/openmp_win_codegen.cpp
  Clang :: OpenMP/parallel_firstprivate_codegen.cpp
  Clang :: OpenMP/parallel_for_reduction_task_codegen.cpp
  Clang :: OpenMP/parallel_master_reduction_task_codegen.cpp
  Clang :: OpenMP/parallel_reduction_task_codegen.cpp
  Clang :: OpenMP/parallel_sections_reduction_task_codegen.cpp
  Clang :: OpenMP/sections_reduction_task_codegen.cpp
  Clang :: OpenMP/target_depend_codegen.cpp
  Clang :: OpenMP/target_enter_data_depend_codegen.cpp
  Clang :: OpenMP/target_exit_data_depend_codegen.cpp
  Clang :: OpenMP/target_parallel_depend_codegen.cpp
  Clang :: OpenMP/target_parallel_for_depend_codegen.cpp
  Clang :: OpenMP/target_parallel_for_reduction_task_codegen.cpp
  Clang :: OpenMP/target_parallel_for_simd_depend_codegen.cpp
  Clang :: OpenMP/target_parallel_reduction_task_codegen.cpp
  Clang :: OpenMP/target_simd_depend_codegen.cpp
  Clang :: OpenMP/target_teams_depend_codegen.cpp
  Clang :: OpenMP/target_teams_distribute_depend_codegen.cpp
  Clang :: OpenMP/target_teams_distribute_parallel_for_depend_codegen.cpp
  Clang :: OpenMP/target_teams_distribute_parallel_for_reduction_codegen.cpp
  Clang :: 
OpenMP/target_teams_distribute_parallel_for_reduction_task_codegen.cpp
  Clang :: OpenMP/target_teams_distribute_parallel_for_simd_depend_codegen.cpp
  Clang :: 
OpenMP/target_teams_distribute_parallel_for_simd_reduction_codegen.cpp
  Clang :: OpenMP/target_teams_distribute_reduction_codegen.cpp
  Clang :: OpenMP/target_teams_distribute_simd_depend_codegen.cpp
  Clang :: OpenMP/target_teams_distribute_simd_reduction_codegen.cpp
  Clang :: OpenMP/target_teams_map_codegen.cpp
  Clang :: OpenMP/target_update_depend_codegen.cpp
  Clang :: OpenMP/teams_distribute_parallel_for_reduction_codegen.cpp
  Clang :: OpenMP/teams_distribute_parallel_for_reduction_task_codegen.cpp
  Clang :: OpenMP/teams_distribute_parallel_for_simd_reduction_codegen.cpp
  Clang :: OpenMP/teams_distribute_reduction_codegen.cpp
  Clang :: OpenMP/teams_distribute_simd_reduction_codegen.cpp
  libomp :: tasking/kmp_taskloop.c
  libomp :: worksharing/for/kmp_doacross_check.c




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:203
+  static Function *getOrCreateRuntimeFunction(Module ,
+  omp::RuntimeFunction FnID);
 

jdoerfert wrote:
> Nit: M is the commonly used name for a module ;)
The OpenMPIRBuilder struct has a Module named M so I wasn't sure if I should be 
shadowing it.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:370
 __OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32,
-  KmpCriticalNamePtrTy, Int32)
+  KmpCriticalNamePtrTy, /* int32? */ Int32)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32,

jdoerfert wrote:
> What about the comments with a `?`?
That's just where I was unsure because I saw some conflicting information, like 
this one with Int32 in the runtime and uintptr_t in Clang.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:103
+  }
+} else if (Fn->getName() == "__kmpc_fork_teams") {
+  if (!Fn->hasMetadata(LLVMContext::MD_callback)) {

jdoerfert wrote:
> Please use `FnID` for the comparison.
Don't know why I didn't think of doing that, will do.


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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

We pass all clang tests, right?




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:203
+  static Function *getOrCreateRuntimeFunction(Module ,
+  omp::RuntimeFunction FnID);
 

Nit: M is the commonly used name for a module ;)



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:370
 __OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32,
-  KmpCriticalNamePtrTy, Int32)
+  KmpCriticalNamePtrTy, /* int32? */ Int32)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32,

What about the comments with a `?`?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:83
 IsVarArg), 
\
-  GlobalValue::ExternalLinkage, Str, M);   
\
+  GlobalValue::ExternalLinkage, Str, Md);  
\
 break;

`M`



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:103
+  }
+} else if (Fn->getName() == "__kmpc_fork_teams") {
+  if (!Fn->hasMetadata(LLVMContext::MD_callback)) {

Please use `FnID` for the comparison.


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

https://reviews.llvm.org/D80222



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 created this revision.
jhuber6 added a reviewer: jdoerfert.
jhuber6 added projects: OpenMP, clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1.
jhuber6 edited the summary of this revision.

This patch changes Clang to generate runtime functions from the information 
inside OMPKinds.def and to use the enumeration values in OMPConstants. Testing 
currently fails because of incomplete type support for 32 and 64 bit targets.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80222

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/test/Transforms/OpenMP/add_attributes.ll

Index: llvm/test/Transforms/OpenMP/add_attributes.ll
===
--- llvm/test/Transforms/OpenMP/add_attributes.ll
+++ llvm/test/Transforms/OpenMP/add_attributes.ll
@@ -491,7 +491,7 @@
 
 declare void @__kmpc_critical(%struct.ident_t*, i32, [8 x i32]*)
 
-declare void @__kmpc_critical_with_hint(%struct.ident_t*, i32, [8 x i32]*, i32)
+declare void @__kmpc_critical_with_hint(%struct.ident_t*, i32, [8 x i32]*, i64)
 
 declare void @__kmpc_end_critical(%struct.ident_t*, i32, [8 x i32]*)
 
@@ -591,19 +591,19 @@
 
 declare i32 @__kmpc_cancellationpoint(%struct.ident_t*, i32, i32)
 
-declare void @__kmpc_push_num_teams(%struct.ident_t*, i32, i32, i32)
+declare i32 @__kmpc_push_num_teams(%struct.ident_t*, i32, i32, i32)
 
 declare void @__kmpc_fork_teams(%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...)
 
 declare void @__kmpc_taskloop(%struct.ident_t*, i32, i8*, i32, i64*, i64*, i64, i32, i32, i64, i8*)
 
-declare i8* @__kmpc_omp_target_task_alloc(%struct.ident_t*, i32, i32, i64, i64, i8*, i64)
+declare i8* @__kmpc_omp_target_task_alloc(%struct.ident_t*, i32, i32, i64, i64, i32 (i32, i8*)*, i64)
 
 declare i8* @__kmpc_taskred_modifier_init(i8*, i32, i32, i32, i8*)
 
 declare i8* @__kmpc_taskred_init(i32, i32, i8*)
 
-declare void @__kmpc_task_reduction_modifier_fini(i8*, i32, i32)
+declare void @__kmpc_task_reduction_modifier_fini(%struct.ident_t*, i32, i32)
 
 declare void @__kmpc_copyprivate(%struct.ident_t*, i32, i64, i8*, void (i8*, i8*)*, i32)
 
@@ -623,6 +623,32 @@
 
 declare void @__kmpc_free(i32, i8*, i8*)
 
+declare void @__kmpc_push_target_tripcount(i64, i64)
+
+declare i32 @__tgt_target(i64, i8*, i32, i8**, i8**, i64*, i64*)
+
+declare i32 @__tgt_target_nowait(i64, i8*, i32, i8**, i8**, i64*, i64*)
+
+declare i32 @__tgt_target_teams(i64, i8*, i32, i8**, i8**, i64*, i64*, i32, i32)
+
+declare void @__tgt_register_requires(i64)
+
+declare void @__tgt_target_data_begin(i64, i32, i8**, i8**, i64*, i64*)
+
+declare void @__tgt_target_data_begin_nowait(i64, i32, i8**, i8**, i64*, i64*)
+
+declare void @__tgt_target_data_end(i64, i32, i8**, i8**, i64*, i64*)
+
+declare void @__tgt_target_data_end_nowait(i64, i32, i8**, i8**, i64*, i64*)
+
+declare void @__tgt_target_data_update(i64, i32, i8**, i8**, i64*, i64*)
+
+declare void @__tgt_target_data_update_nowait(i64, i32, i8**, i8**, i64*, i64*)
+
+declare i64 @__tgt_mapper_num_components(i8*)
+
+declare void @__tgt_push_mapper_component(i8*, i8*, i8*, i64, i64)
+
 declare i8* @__kmpc_task_allow_completion_event(%struct.ident_t*, i32, i8*)
 
 declare i8* @__kmpc_task_reduction_get_th_data(i32, i8*, i8*)
@@ -904,7 +930,7 @@
 ; CHECK-NEXT: declare void @__kmpc_critical(%struct.ident_t*, i32, [8 x i32]*)
 
 ; CHECK: Function Attrs: inaccessiblemem_or_argmemonly
-; CHECK-NEXT: declare void @__kmpc_critical_with_hint(%struct.ident_t*, i32, [8 x i32]*, i32)
+; CHECK-NEXT: declare void @__kmpc_critical_with_hint(%struct.ident_t*, i32, [8 x i32]*, i64)
 
 ; CHECK: Function Attrs: inaccessiblemem_or_argmemonly
 ; CHECK-NEXT: declare void @__kmpc_end_critical(%struct.ident_t*, i32, [8 x i32]*)
@@ -1054,7 +1080,7 @@
 ; CHECK-NEXT: declare i32 @__kmpc_cancellationpoint(%struct.ident_t*, i32, i32)
 
 ; CHECK: Function Attrs: nounwind
-; CHECK-NEXT: declare void @__kmpc_push_num_teams(%struct.ident_t*, i32, i32, i32)
+; CHECK-NEXT: declare i32 @__kmpc_push_num_teams(%struct.ident_t*, i32, i32, i32)
 
 ; CHECK: Function Attrs: nounwind
 ; CHECK-NEXT: declare void @__kmpc_fork_teams(%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...)
@@ -1063,7 +1089,7 @@
 ; CHECK-NEXT: declare void @__kmpc_taskloop(%struct.ident_t*, i32, i8*, i32, i64*, i64*, i64, i32, i32, i64, i8*)
 
 ; CHECK: Function Attrs: nounwind
-; CHECK-NEXT: declare i8* @__kmpc_omp_target_task_alloc(%struct.ident_t*, i32, i32, i64, i64, i8*, i64)
+; CHECK-NEXT: declare i8* @__kmpc_omp_target_task_alloc(%struct.ident_t*, i32, i32, i64, i64, i32 (i32, i8*)*, i64)
 
 ; CHECK: Function Attrs: nounwind
 ; CHECK-NEXT: declare i8* @__kmpc_taskred_modifier_init(i8*, i32, i32, i32, i8*)
@@ -1072,7 +1098,7 @@
 ; CHECK-NEXT: declare i8* @__kmpc_taskred_init(i32, i32, i8*)
 
 ; CHECK: Function Attrs: nounwind
-; CHECK-NEXT: declare void