tpp-mlir icon indicating copy to clipboard operation
tpp-mlir copied to clipboard

Handle OpenMP libraries in portable way (issue #959)

Open dbabokin opened this issue 1 year ago • 6 comments

Closes #959

See the issue for detailed problem description.

dbabokin avatar Aug 29 '24 20:08 dbabokin

Looks like gcc has trouble with these changes.

adam-smnk avatar Aug 30 '24 14:08 adam-smnk

I can reproduce it. The problem is that with gcc it pulls in gcc's libgomp.so.1 instead of llvm's libomp.so.5. 🤔

dbabokin avatar Aug 30 '24 17:08 dbabokin

I see no way to specify for find_package(OpenMP) that it needs llvm version. Not sure how to fix that properly.

dbabokin avatar Aug 30 '24 17:08 dbabokin

The way we were doing it was not portable "for a reason": we don't want to use Gomp because the performance isn't good.

I'm not saying it was done in the right way, clearly not. But the reason is still important.

So, my tip for this PR is to find a way to make it portable, but still require LLVM's OpenMP library and not the compiler's own.

CMake find_package can be fiddled with HINTs and other flags, maybe this helps?

rengolin avatar Sep 04 '24 08:09 rengolin

The way we were doing it was not portable "for a reason": we don't want to use Gomp because the performance isn't good.

Yeah, totally understand. Showing up of gomp when gcc was used was absolutely unexpected. The definition of "portable" that seems acceptable in this case is that it works on macOS and Ubuntu 24.04 - all with llvm's omp.

I'm not saying it was done in the right way, clearly not. But the reason is still important.

So, my tip for this PR is to find a way to make it portable, but still require LLVM's OpenMP library and not the compiler's own.

CMake find_package can be fiddled with HINTs and other flags, maybe this helps?

HINT is a Windows thing, PATH probably may help, I'll give it a try. I'll also file an issue for CMake to get it fixed in the long run.

dbabokin avatar Sep 04 '24 15:09 dbabokin

I file CMake issue for that: https://gitlab.kitware.com/cmake/cmake/-/issues/26263

dbabokin avatar Sep 04 '24 16:09 dbabokin

I'm closing this in favour of #984 which should work around the fact that we want LLVM's version.

The answer in the CMake issue you reported means to me they won't try to make this better in any way.

rengolin avatar Nov 21 '24 10:11 rengolin

Just updated to Ubuntu 24.04 and got the same problem reported here, and the PR here does fix the issue. I'll reopen this and will test on other environments. Since we don't run benchmarks with GCC, I think it will be fine.

rengolin avatar Mar 03 '25 16:03 rengolin

The problem with GCC tests is:

JIT session error: Symbols not found: [ __kmpc_barrier, __kmpc_for_static_fini, __kmpc_for_static_init_8u, __kmpc_fork_call, __kmpc_global_thread_num ]
Error: Failed to materialize symbols: { (main, { entry, _mlir__entry..omp_par, _mlir_entry, _mlir__entry, _entry }) }

rengolin avatar Mar 03 '25 17:03 rengolin

#1019 superseeds it.

rengolin avatar Mar 04 '25 10:03 rengolin