This is an archive of the discontinued LLVM Phabricator instance.

[clang-offload-bundler] Add option -allow-missing-bundles
ClosedPublic

Authored by yaxunl on Dec 10 2020, 1:33 PM.

Details

Summary

There are out-of-tree tools using clang-offload-bundler to extract
bundles from bundled files. When a bundle is not in the bundled
file, clang-offload-bundler is expected to emit an error message
and return non-zero value. However currently clang-offload-bundler
silently generates empty file for the missing bundles.

Since OpenMP/HIP toolchains expect the current behavior, an option
-allow-missing-bundles is added to let clang-offload-bundler
create empty file when a bundle is missing when unbundling.
The unbundling job action is updated to use this option by
default.

clang-offload-bundler itself will emit error when a bundle
is missing when unbundling by default.

Changes are also made to check duplicate targets in -targets
option and emit error.

Diff Detail

Event Timeline

yaxunl requested review of this revision.Dec 10 2020, 1:33 PM
yaxunl created this revision.
tra added a comment.Dec 10 2020, 2:56 PM
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
980

Do we need to report complete list of missing bundles?
Can we just report the first one we've encountered and abort the command?

985

This assumes that all items on the WorkList were unique.
If some of the WorkList items were duplicated, then there will be fewer items in Sorted and the I == Last comparison will never be true.
I'd use Sorted.size() instead.

998–999

Can we, instead use an explicit option to enable this 'make empty files for missing targets' and report missing targets as an error otherwise?
I would assume that missing a target explicitly specified on command line would be an error more often than not. After all we've asked to extract something which implies that that 'something' exists. I.e. if I tell unzip to unpack a non-existing file from an archive, I do not expect it to give me an empty file.

yaxunl marked 3 inline comments as done.Dec 10 2020, 7:45 PM
yaxunl added inline comments.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
980

Better to report all missing bundles since the info is available here and useful.

985

clang-offload-bundler does not allow duplicated targets. It asserts when duplicated targets in options are found when unbundling. Will add check for duplicate targets in options and emit error.

998–999

will do

yaxunl updated this revision to Diff 311099.Dec 10 2020, 7:56 PM
yaxunl marked 3 inline comments as done.
yaxunl retitled this revision from [clang-offload-bundler] Add option -fail-on-missing-bundles to [clang-offload-bundler] Add option -allow-missing-bundles.
yaxunl edited the summary of this revision. (Show Details)

Revised by Artem's comments.

tra accepted this revision.Dec 14 2020, 2:37 PM

LGTM.

The patch could use an OK with OMP folks, considering that we've changed the way offload bunder is invoked for OMP.

clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
985

I'd still use Sorted.size() as it's the Sorted you're iterating on.

1126

Nit: .contains(Target) ?

This revision is now accepted and ready to land.Dec 14 2020, 2:37 PM
yaxunl marked 2 inline comments as done.Dec 16 2020, 7:42 AM

@ABataev Is this patch OK for OpenMP? It is NFC for OpenMP toolchain but affects using clang-offload-bundler as a standalone tool. Thanks.

clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
985

will fix

1126

will fix

@ABataev Is this patch OK for OpenMP? It is NFC for OpenMP toolchain but affects using clang-offload-bundler as a standalone tool. Thanks.

Yes, I think it is ok.

This revision was automatically updated to reflect the committed changes.
yaxunl marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2020, 11:53 AM