Page MenuHomePhabricator

[libomptarget][amdcgn] Add build dependency for llvm-link and opt
ClosedPublic

Authored by protze.joachim on Aug 19 2021, 1:09 PM.

Details

Summary

D107156 and D107320 are not sufficient when OpenMP is built as llvm runtime because dependencies only work within the same cmake instance.
We could limit the dependency to cases where libomptarget/plugins are really built. But compared to the whole llvm project, building openmp runtime is negligible and postponing the build of OpenMP runtime after the dependencies are ready seems reasonable.

The direct dependency introduced in D107156 and D107320 is necessary for the case where OpenMP is built as llvm project.
This patch supersedes / includes D107320.

Diff Detail

Event Timeline

protze.joachim created this revision.Aug 19 2021, 1:09 PM
protze.joachim requested review of this revision.Aug 19 2021, 1:09 PM
Herald added a project: Restricted Project. · View Herald Transcript
protze.joachim edited the summary of this revision. (Show Details)Aug 19 2021, 1:10 PM
JonChesterfield accepted this revision.Aug 19 2021, 1:52 PM

Thank you very much for getting to the bottom of this! One to add to the llvm-13 branch too I think

This revision is now accepted and ready to land.Aug 19 2021, 1:52 PM

Thank you very much for getting to the bottom of this! One to add to the llvm-13 branch too I think

Does the documentation for the release suggest to use LLVM_ENABLE_RUNTIME=openmp rather than LLVM_ENABLE_PROJECT=openmp?

JonChesterfield added a comment.EditedAug 19 2021, 2:15 PM

I think we've written ENABLE_RUNTIMES as the recommended approach in some of the docs but don't know if those correspond to what the release uses. E.g. https://openmp.llvm.org/docs/SupportAndFAQ.html suggests ENABLE_RUNTIMES.

This revision was landed with ongoing or failed builds.Aug 19 2021, 4:59 PM
This revision was automatically updated to reflect the committed changes.

During further testing. I found that nvptx-new had a similar issue. I also had to move the depend code in the runtime cmake file, but I could verify in the ninja.build file, that this change now really adds the expected necessary dependency.

Reading the ninja file to determine what cmake's dependency graph looked like is a great idea. Going to use that in future, thanks for the comment!

Reading the ninja file to determine what cmake's dependency graph looked like is a great idea. Going to use that in future, thanks for the comment!

(drive-by comment unrelated to the patch, but because of "reading ninja files")

You know about Ninja's "Extra Tools", in particular ninja -t browse and ninja -t graph? That should be a nicer UX than going through the machine-readable build.ninja 😉