Page MenuHomePhabricator

[OpenMP] Deprecate the old driver for OpenMP offloading
ClosedPublic

Authored by jhuber6 on Jul 18 2022, 8:33 AM.

Details

Summary

Recently OpenMP has transitioned to using the "new" driver which
primarily merges the device and host linking phases into a single
wrapper that handles both at the same time. This replaced a few tools
that were only used for OpenMP offloading, such as the
clang-offload-wrapper and clang-nvlink-wrapper. The new driver
carries some marked benefits compared to the old driver that is now
being deprecated. Things like device-side LTO, static library
support, and more compatible tooling. As such, we should be able to
completely deprecate the old driver, at least for OpenMP. The old driver
support will still exist for CUDA and HIP, although both of these can
currently be compiled on Linux with --offload-new-driver to use the new
method.

Note that this does not deprecate the clang-offload-bundler, although
it is unused by OpenMP now, it is still used by the HIP toolchain both
as their device binary format and object format.

When I proposed deprecating this code I heard some vendors voice
concernes about needing to update their code in their fork. They should
be able to just revert this commit if it lands.

Diff Detail

Unit TestsFailed

TimeTest
30 msx64 debian > Clang.Driver::clang-offload-wrapper.c
Script: -- : 'RUN: at line 6'; clang-offload-wrapper --help | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/clang-offload-wrapper.c --check-prefix CHECK-HELP

Event Timeline

jhuber6 created this revision.Jul 18 2022, 8:33 AM
jhuber6 requested review of this revision.Jul 18 2022, 8:33 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 18 2022, 8:33 AM

I noticed that in one of my applications, offload to x86 is not fully working with static libraries but directly linking all object files resolves the issue. So the new driver doesn't cause regression compared to the old driver which doesn't work with static libraries anyway. So we can say good bye to the old drivers.

jdoerfert accepted this revision.Aug 24 2022, 7:42 AM

LG, as discussed over many weeks in our calls.

This revision is now accepted and ready to land.Aug 24 2022, 7:42 AM
ye-luo accepted this revision.Aug 24 2022, 8:03 AM

The new driver remains failing offload to x86 in certain scenarios when linking static libraries. Once I link object files directly there is no issue. This not worse than old driver that only supports linking object files directly.

jhuber6 updated this revision to Diff 455233.Aug 24 2022, 8:34 AM

Remove old offload wrapper test.

MaskRay accepted this revision.Aug 24 2022, 9:13 AM

This is "remove" instead of "deprecate"?

This is "remove" instead of "deprecate"?

Yes, "remove" is probably more appropriate as this completely removes support.

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Aug 26 2022, 12:11 PM

This caused:

In file included from ../../clang/lib/Driver/ToolChains/Cuda.cpp:9:
../../clang/lib/Driver/ToolChains/Cuda.h:193:29: warning: private field 'OK' is not used [-Wunused-private-field]
  const Action::OffloadKind OK;
                            ^

This caused:

In file included from ../../clang/lib/Driver/ToolChains/Cuda.cpp:9:
../../clang/lib/Driver/ToolChains/Cuda.h:193:29: warning: private field 'OK' is not used [-Wunused-private-field]
  const Action::OffloadKind OK;
                            ^

Thanks for bringing this to my attention, it should be an easy fix.

ps: Thank you to everyone who's been working on having fewer offload wrapper binaries!