Details
- Reviewers
jdoerfert - Commits
- rGe007b3286429: [OpenMP] Add time profiling for libomptarget
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Interesting. Not sure about the dependency on an llvm library, though maybe guarding it makes that acceptable.
The DIY equivalent we've been using is functionally similar. Implemented at trace.h in as unintrusive a fashion as I could manage.
In addition to the time taken by calls, maintaining a log of the arguments to the functions (and the return value) has been very helpful as a debugging hack. The proper thing to do is probably to write said data to an in memory ring buffer. The above header just splats it to stdout.
Does this clang-format nicely? I'd expect to have to spell it TIMESCOPE(); or similar.
I left two comments that we should address now, a third can be future work and the rest has already nice TODOs in the code. Thanks for working on this so quickly.
openmp/libomptarget/src/CMakeLists.txt | ||
---|---|---|
40 | Add a CMAKE flag to disable this, by default it can be enabled though. | |
openmp/libomptarget/src/api.cpp | ||
21 | Maybe we could move this in one of the header, private or rtl? We might even later use it for internal functions. | |
openmp/libomptarget/src/interface.cpp | ||
130 | We should pass loc to TIMESCOPE. |
openmp/libomptarget/src/api.cpp | ||
---|---|---|
21 | After looking at the output in a flamegraph, I'm not sure if we should not go with __FUNCTION__ instead. Unclear if the types help in any way. |
openmp/libomptarget/src/api.cpp | ||
---|---|---|
16–21 | Then use it like: TIMESCOPE(); ? |
Thanks for the comment Jon. It's interesting to include debugging information. I have changed the macro to the format you suggest.
openmp/libomptarget/src/interface.cpp | ||
---|---|---|
130 | Left that for future work |
openmp/libomptarget/src/api.cpp | ||
---|---|---|
16–21 | Thanks! That's indeed much nicer. |
LGTM, I left two minor change requests below. Thanks for getting this up and running so quickly!
openmp/libomptarget/src/CMakeLists.txt | ||
---|---|---|
41 | Move the conditional if(OPENMP_ENABLE_LIBOMPTARGET_PROFILING) to include all of the stuff, including the linking of things. So if it is disabled we build as we did before. | |
openmp/libomptarget/src/rtl.cpp | ||
37 | Make this a char* and set it to the value of LIBOMPTARGET_PROFILE. We will assume that is a path for the resulting file. |
I think there is something wrong in this patch
It creates a new library called:
usr/lib/llvm-12/lib/libomptarget.so.12
When current files are:
/usr/lib/llvm-12/lib/libomp.so.5
So, at least for consistency, the lib should be called libomptarget.so.12
@ggeorgakoudis should I report a new bug? thanks
There's no change to the cmake files that control the output file name in this patch so I don't think this can be the cause of the change you're seeing. There may be an existing bug about how the libraries should be named.
openmp/CMakeLists.txt | ||
---|---|---|
86 | So is it intentional to enable profiling as long as libomptarget is enabled? |
openmp/CMakeLists.txt | ||
---|---|---|
86 | Yes, profiling is by default a single thread local memory access per entry point. If it turns out to be a problem we can revisit this. |
@ggeorgakoudis In our local build environment I am now observing error below:
FAILED: lib/libomptarget.so.12git
..
/home/engshare/third-party2/binutils/2.32/centos7-native/da39a3e/bin/ld.gold: error: lib/libLLVMSupport.a(TimeProfiler.cpp.o): requires dynamic R_X86_64_32 reloc which may overflow at runtime; recompile with -fPIC
We are building llvm with -DLLVM_ENABLE_PIC=OFF.
So with ENABLE_LIBOMPTARGET enabling OPENMP_ENABLE_LIBOMPTARGET_PROFILING it goes to else case and includes Support library, which leads to link failure.
Maybe add explicit flag NOT_LINK_SUPPORT or something like that for this case, or break up the ENABLE_LIBOMPTARGET/OPENMP_ENABLE_LIBOMPTARGET_PROFILING dependency?
Ah, yes. That makes sense. Building llvmsupport without PIC is a reasonable thing to do, and will cause problems when linked into libomptarget. One solution is to link libomptarget against a dynamically built llvmsupport, which might mean building llvmsupport twice. I don't know whether the existing cmake is wired up to do that.
I'd prefer we default to profiling disabled (and the llvm libs not linked in) for now.
(actually my preferred solution is to link libomptarget statically, but that feels like a longer term goal)
Why add a new flag if you can disable profiling with the one added? I mean, OPENMP_ENABLE_LIBOMPTARGET_PROFILING=OFF should unbreak your build, correct?
So far there was no convincing reason why it should be off. Having it on by default has the distinct advantage that most people get the feature and can use profiling with their local build or release of LLVM, that is arguably useful. If you start with some fancy build you might need to disable it, or upstream support of your fancy build, neither is unprecedented.
See above.
FYI, libomptarget LIT tests fail with profiling enabled in shared-libs builds, because libLLVMSupport.so.12git dependency cannot be resolved. Apparently, LD_LIBRARY_PATH is only set to <build>/llvm/projects/openmp/libomptarget, which obviously does not contain the support library.
@vzakhari Is it an out-of-tree build? If it is, you need to set LIBOMPTARGET_OPENMP_HEADER_FOLDER and DLIBOMPTARGET_OPENMP_HOST_RTL_FOLDER (https://github.com/llvm/llvm-project/tree/main/openmp#id11).
No, it is an in-tree build. I see libomptarget's output location (LIBOMPTARGET_LIBRARY_DIR) does not align well with the default rpath setting done inside add_llvm_library.
Hi @ggeorgakoudis, please try something along these lines:
cmake -G "Unix Makefiles" -DLLVM_ENABLE_PROJECTS="clang;compiler-rt;openmp" -DCMAKE_COLOR_MAKEFILE=0 -DBUILD_SHARED_LIBS=1 -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD="X86" -DCMAKE_C_COMPILER=".../bin/gcc" -DCMAKE_CXX_COMPILER=".../bin/g++" -DLLVM_BINUTILS_INCDIR=".../lib/gcc/x86_64-linux-gnu/7.3.0/plugin/include" -DCMAKE_INSTALL_PREFIX="..." ".../llvm"
make install
make check-libomptarget
openmp/libomptarget/src/CMakeLists.txt | ||
---|---|---|
35 | Hello. This function adds a LLVM_VERSION_MAJOR suffix to libomptarget library name, and libomptarget.so is just a link now. This actually causes dynamic dependency in the executables to be set to libomptarget.so.##git. When LLVM changes its major version, I guess all the executables built with the previous version will stop working. Is this expected or I am missing something? |
openmp/libomptarget/src/CMakeLists.txt | ||
---|---|---|
35 | The way I understand you this seems desirable. |
openmp/libomptarget/src/CMakeLists.txt | ||
---|---|---|
35 | I am not sure why we want to make libomptarget versioning dependent on llvm versioning. We used to have backward compatible libomptarget, i.e. an executable built with llvm12 libomptarget could work with llvm13 libomptarget even if there was a breaking LLVM IR change between these two versions of llvm. Now we disallow this, but it does not seem like this was the main goal of this commit. I bthink if we want to introduce versioning for libomptarget it should be independent of llvm versioning. |
So is it intentional to enable profiling as long as libomptarget is enabled?