This is an archive of the discontinued LLVM Phabricator instance.

[clang] Search runtimes build tree for openmp runtime
Changes PlannedPublic

Authored by JonChesterfield on May 5 2021, 11:52 AM.

Details

Summary

D96248 added support for finding the openmp devicertl bitcode in the clang
lib directory where it is installed, or in the LIBRARY_PATH environment.

When testing openmp from a build using ENABLE_RUNTIMES, the corresponding
bitcode may be found under runtimes/runtimes-bins. This patch adds that to
the list of places clang looks, at lower priority then the others.

Diff Detail

Unit TestsFailed

Event Timeline

JonChesterfield requested review of this revision.May 5 2021, 11:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2021, 11:52 AM
JonChesterfield added a comment.EditedMay 5 2021, 2:46 PM

I think the lit test suite is intended to set environment variables such that this is not necessary when running the tests.

edit: This patch resulted from some confusion on my part. Given a list test that fails, copy&paste the failing RUN line into a shell fails to find the device runtime, because the environment setup done by lit is not reflected in the printed RUN line.

That seems to be a drawback from using environment variables to tie the test cases together. We could emit LIBRARY_PATH= lines in front of RUN, but that is probably a cross-OS hazard.

tianshilei1992 requested changes to this revision.May 11 2021, 8:05 AM

From my perspective, this is not a good direction. No need to bother Clang driver for one case of testing. LIBRARY_PATH needs to be set correctly by lit instead of hardcoded the logic in the driver.

This revision now requires changes to proceed.May 11 2021, 8:05 AM
JonChesterfield added a comment.EditedMay 11 2021, 8:20 AM

More compelling than the testing case is that openmp is presently unusable without setting ld_library_path. This is part of fixing that.

Edit: got this confused with one of the other patches. This one is only valuable if the other patches to drop library path land.

Unlike others, such as /usr/local/lib or /usr/local/cuda, it is not a standard place. It doesn't make too much sense to hardcode it in the driver.

I'm not so sure about that. With the other stuff in flight, it would let us build openmp apps from the clang build directory. That's useful for quicker build/debug cycles, as well as for lit tests that can be run by copying into a shell. At zero runtime cost when the library is found elsewhere, and trivial extra code in the driver.

JonChesterfield edited the summary of this revision. (Show Details)May 25 2021, 3:18 PM

At present libomptarget_amdgcn_bc_path and nvptx need to be a path to a file. If we relax that to accept a path to a directory in which the corresponding file is found, then we can use that argument in place of LIBRARY_PATH from the test scripts. That will let the tests run, gives developers absolute control over which deviceRTL library is linked in, stops clang from picking up the wrong file from an unrelated toolchain on LIBRARY_PATH. I consider that strictly better than the above patch. Thoughts?

ye-luo added a subscriber: ye-luo.Aug 30 2021, 8:43 PM

It seems that this path is baked in to clang executable even after make install. I'm not convinced this is the right direction.

JonChesterfield planned changes to this revision.Aug 30 2021, 11:22 PM

Yeah, we probably can't leave the installed program with a search path only used for testing. I'm planning to revisit this to use libomptarget_amdgcn_bc_path instead of LIBRARY_PATH for testing, then drop the LIBRARY_PATH override.