ABataev added inline comments.
================
Comment at: clang/test/Driver/clang-offload-bundler.c:228-231
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=ast
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1
-inputs=%t.bundle3.unordered.ast -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
----------------
What about tests for other kinds of elements like preprocessed code, IR,
objects, etc.?
================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:888
+ // treat missing host triple as error if we do unbundling.
+ if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
Error = true;
----------------
sdmitriev wrote:
> sdmitriev wrote:
> > sdmitriev wrote:
> > > ABataev wrote:
> > > > sdmitriev wrote:
> > > > > ABataev wrote:
> > > > > > I believe, for unbundling we also must check for `!= 1` rather
> > > > > > than `> 1`. Zero host targets also is not allowed.
> > > > > But the whole idea of this change is to remove requirement to provide
> > > > > host triple for unbundling operation. Target bundle(s) can always be
> > > > > extracted without extracting host, so host bundle is optional.
> > > > > Therefore zero host targets should not be considered as error for
> > > > > unbundling.
> > > > And why do we need this? I think it would be better to check that the
> > > > requested host triple matches the bundled one using this parameter
> > > > rather than removing it.
> > > > And why do we need this?
> > >
> > > As I wrote in the summary it is a usability issue. You may for example
> > > want to extract device object for a particular offload target to examine
> > > its contents (symbols, sections, etc..), but currently you also have to
> > > extract host bundle as well even if you do not need it.
> > >
> > > > I think it would be better to check that the requested host triple
> > > > matches the bundled one using this parameter rather than removing it.
> > >
> > > So you suggest to check that host bundle name that exists in the fat
> > > image matches the host bundle name provided it command line if it was
> > > provided? Should it be an error if names do not match?
> > >
> > I have updated patch to do error checking if host bundle name was provided
> > in command line.
> @ABataev I believe the host bundle name is now being checked as you
> suggested. Can you please confirm that it matches your expectations?
Ok, I got the idea of the patch. BTW, will happen if I request the device code
for the triple, which is correct, but bundled container does not have the
device code for this triple?
================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:795
+ // in case host bundle name was provided in command line.
+ if (!FoundHostBundle && HostInputIndex != ~0u) {
errs() << "error: Can't find bundle for the host target\n";
----------------
There must be a test for this change. better to implement it in a different
patch, I think.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
