OP-DSL / OP2-Common

OP2: open-source framework for the execution of unstructured grid applications on clusters of GPUs or multi-core CPUs
https://op-dsl.github.io
Other
99 stars 47 forks source link

Issues when building an OP2 library and then linking to a OP2 application #235

Closed TobyFlynn closed 1 year ago

TobyFlynn commented 1 year ago

There are a couple of issues that I have noticed when building a library that uses OP2 which I then link to a separate OP2 application. Both of the issues are easy to work around so they aren't pressing issues.

The kernel records (i.e. what keeps track of the kernels' wall time and other metrics) of the library and application conflict. From what I understand, the OP2 code gen assigns each OP2 kernel an index in the global array of OP_kernels but some kernels in the OP2 library will end up with the same index as kernels in the OP2 application.

For the CUDA code gen, there is an issue with declaring OP2 constants. After performing the OP2 code gen, both the library and the application provide implementations for op_decl_const_char which will conflict, leading to only one declaring its constants correctly.

I'm happy to have a go at solving the above but I think I'll get in the way of the new code gen development if I try and change anything at the moment

reguly commented 1 year ago

Just to clarify, the library itself is like an OP2 application, with kernels, a code generation step, etc., then the application once again doing code generation, etc?If so, that's tricky - one can of course fix the counters for the various kernel fairly easily. But I am not so sure about all the decl const stuff, I can think of a range of things to try. On 2023. Feb 7., at 10:33, Toby Flynn @.***> wrote: There are a couple of issues that I have noticed when building a library that uses OP2 which I then link to a separate OP2 application. Both of the issues are easy to work around so they aren't pressing issues. The kernel records (i.e. what keeps track of the kernels' wall time and other metrics) of the library and application conflict. From what I understand, the OP2 code gen assigns each OP2 kernel an index in the global array of OP_kernels but some kernels in the OP2 library will end up with the same index as kernels in the OP2 application. For the CUDA code gen, there is an issue with declaring OP2 constants. After performing the OP2 code gen, both the library and the application provide implementations for op_decl_const_char which will conflict, leading to only one declaring its constants correctly. I'm happy to have a go at solving the above but I think I'll get in the way of the new code gen development if I try and change anything at the moment

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

bozbez commented 1 year ago

The op-cg Fortran generator generates op_decl_const_x(...) (where x is the const name) for each const, perhaps it's worth bringing this behavior over to the C++ generator as well... this would only work if the library and application do not share consts though?

reguly commented 1 year ago

That’s a good approach. I think trying to declare two global variables with the same name would also cause a linker error, so this restriction should be fine.

On 2023. Feb 7., at 13:11, Joe Kaushal @.***> wrote:

The op-cg Fortran generator generates op_decl_const_x(...) (where x is the const name) for each const, perhaps it's worth bringing this behavior over to the C++ generator as well... this would only work if the library and application do not share consts though?

— Reply to this email directly, view it on GitHub https://github.com/OP-DSL/OP2-Common/issues/235#issuecomment-1420672713, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJWVVMEJGX3SJOOUX6N4BDWWI3WZANCNFSM6AAAAAAUTZAJRM. You are receiving this because you commented.

TobyFlynn commented 1 year ago

Yes the library is just like an OP2 application with its own kernels and separate code gen step. The op_decl_const_x sounds like a good fix for this, thanks!

reguly commented 1 year ago

@TobyFlynn Please check if all work OK