Differential D109061
[openmp] No longer use LIBRARY_PATH to find devicertl Authored by JonChesterfield on Sep 1 2021, 6:29 AM.
Details Given D109057, change test runner to use the libomptarget-x-bc-path Also drop the use of LIBRARY_PATH environment variable as it is far
Diff Detail
Unit Tests Event TimelineComment Actions I suppose it require users to add a command line argument every time they want to build a program? Comment Actions Nope, works the same as it used to - looks relative to clang. However I think it's broken some CI machines Comment Actions From my perspective, using LIBRARY_PATH to find bitcode library conforms with convention because the library is linked in a linkage during compilation time, where LIBRARY_PATH is used by the linker. If we don't want to mess up test results, the patch to force rpath should do the right thing. Comment Actions Using LIBRARY_PATH to find the bitcode is a reasonable interpretation of LIBRARY_PATH but it's a disaster in terms of using clang on a system that has LIBRARY_PATH pointing at some other version of clang. Rpath will be ignored by the bitcode lib handling. How does this break under standalone? It uses the same config.variable that is used for LIBRARY_PATH at the moment, so it looks in exactly the same place as before this patch Comment Actions OpenMP libraries don't always sit next to clang's default library path. For standalone build, the installation path of OpenMP can be different from that of LLVM. Before this patch, clang can find the library as long as openmp/lib is in LIBRARY_PATH. Now it can never find it except we pass the path explicitly to the compilation command line. I understand your argument that clang might find a wrong one, but it's user's responsibility to make sure the expected version should be on the top of LIBRARY_PATH. Comment Actions That helps disambiguate. You aren't concerned that this change will break the in tree tests, rather that people who currently rely on LIBRARY_PATH to choose a bitcode library to use with a clang located somewhere else in the filesystem will now need to pass --libomptarget-nvptx-bc-path instead, or put a symlink relative to clang. That is a huge win for anyone who has LIBRARY_PATH pointing to one toolchain and who wants to be able to use clang from another as it replaces (non-diagnosed) runtime misbehaviour with a compile time error about being unable to find the devicertl. E.g. if they've had to set LIBRARY_PATH to find something their application uses that is unrelated to that clang, perhaps a vendor toolchain. It's also a moderate win in that it hopefully dissuades people from assuming that any clang can be used with any openmp offloading library, which is very much not true. It breaks the use case of people who have an exactly matching clang and openmp, that they installed in unrelated places, and use LIBRARY_PATH to stitch back together because they don't have to use it for anything else. They get to use the commandline arguments instead. I think that's great all around. It makes it much less likely that people will mismatch clang and devicertl, which is good for UX and for our bug handling. If we need to preserve the environment plumbing then let's have a new variable named for the purpose, so that it doesn't collide with LIBRARY_PATH, and still wire tests together with the commandline argument as it lets people copy & paste the failing line into a new terminal to debug it. Comment Actions Sending back to review now that the test updates are included and LIBRARY_PATH removed from the test setup entirely
Comment Actions In order to tackle the issues, I think we can do it this way: put the clang default lib ahead of LIBRARY_PATH. This can make sure clang always uses the bitcode library that comes with it, and if it is not there, it also has the ability to find a potential one from LIBRARY_PATH. Comment Actions @tianshilei1992 can we agree that the tests must point to a specific deviceRTL, and not dig one out of LIBRARY_PATH? That can be factored out of this patch to make the tests always use exactly the library that was just built, and also makes them easier to run outside of the test harness. Comment Actions Discussed at weekly call. Going to go with the priority order: That means people who have installed clang+openmp together have something that works out of the box, and they can override the deviceRTL if they wish. It also means people who have installed matching clang/openmp in different locations and wired them together with LIBRARY_PATH will continue to work. I'll amend the above to do that. Comment Actions For test, IMO we could do whatever we want. That’s why I accepted the patch regarding rpath.
| ||||||||||||||||||||||||
This part is contained in D109057 which is intended to land first