[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
This revision was automatically updated to reflect the committed changes. Closed by commit rL370143: [Clang][Bundler] Do not require host triple for extracting device bundles (authored by sdmitriev, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D66601?vs=217522=217558#toc Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66601/new/ https://reviews.llvm.org/D66601 Files: cfe/trunk/test/Driver/clang-offload-bundler.c cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp Index: cfe/trunk/test/Driver/clang-offload-bundler.c === --- cfe/trunk/test/Driver/clang-offload-bundler.c +++ cfe/trunk/test/Driver/clang-offload-bundler.c @@ -156,14 +156,20 @@ // RUN: diff %t.i %t.res.i // RUN: diff %t.tgt1 %t.res.tgt1 // RUN: diff %t.tgt2 %t.res.tgt2 +// RUN: clang-offload-bundler -type=i -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.i -unbundle +// RUN: diff %t.tgt1 %t.res.tgt1 // RUN: clang-offload-bundler -type=ii -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ii,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ii -unbundle // RUN: diff %t.ii %t.res.ii // RUN: diff %t.tgt1 %t.res.tgt1 // RUN: diff %t.tgt2 %t.res.tgt2 +// RUN: clang-offload-bundler -type=ii -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.ii -unbundle +// RUN: diff %t.tgt2 %t.res.tgt2 // RUN: clang-offload-bundler -type=ll -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ll,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ll -unbundle // RUN: diff %t.ll %t.res.ll // RUN: diff %t.tgt1 %t.res.tgt1 // RUN: diff %t.tgt2 %t.res.tgt2 +// RUN: clang-offload-bundler -type=ll -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.ll -unbundle +// RUN: diff %t.tgt1 %t.res.tgt1 // RUN: clang-offload-bundler -type=s -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.s,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.s -unbundle // RUN: diff %t.s %t.res.s // RUN: diff %t.tgt1 %t.res.tgt1 @@ -172,6 +178,8 @@ // RUN: diff %t.s %t.res.s // RUN: diff %t.tgt1 %t.res.tgt1 // RUN: diff %t.tgt2 %t.res.tgt2 +// RUN: clang-offload-bundler -type=s -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.s -unbundle +// RUN: diff %t.tgt2 %t.res.tgt2 // Check if we can unbundle a file with no magic strings. // RUN: clang-offload-bundler -type=s -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.s,%t.res.tgt1,%t.res.tgt2 -inputs=%t.s -unbundle @@ -183,6 +191,10 @@ // RUN: diff %t.empty %t.res.tgt1 // RUN: diff %t.empty %t.res.tgt2 +// Check that bindler prints an error if given host bundle does not exist in the fat binary. +// RUN: not clang-offload-bundler -type=s -targets=host-x86_64-xxx-linux-gnu,openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.s,%t.res.tgt1 -inputs=%t.bundle3.s -unbundle 2>&1 | FileCheck %s --check-prefix CK-NO-HOST-BUNDLE +// CK-NO-HOST-BUNDLE: error: Can't find bundle for the host target + // // Check binary bundle/unbundle. The content that we have before bundling must be the same we have after unbundling. // @@ -194,10 +206,14 @@ // RUN: diff %t.bc %t.res.bc // RUN: diff %t.tgt1 %t.res.tgt1 // RUN: diff %t.tgt2 %t.res.tgt2 +// RUN: clang-offload-bundler -type=bc -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.bc -unbundle +// RUN: diff %t.tgt1 %t.res.tgt1 // RUN: clang-offload-bundler -type=gch -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.gch,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.gch -unbundle // RUN: diff %t.ast %t.res.gch // RUN: diff %t.tgt1 %t.res.tgt1 // RUN: diff %t.tgt2 %t.res.tgt2 +// RUN: clang-offload-bundler -type=gch -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.gch -unbundle +// RUN: diff %t.tgt2 %t.res.tgt2 // RUN: clang-offload-bundler -type=ast -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ast,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ast -unbundle // RUN: diff %t.ast %t.res.ast // RUN: diff %t.tgt1 %t.res.tgt1 @@ -210,6 +226,8 @@ // RUN: diff %t.ast %t.res.ast // RUN: diff %t.tgt1 %t.res.tgt1 // RUN: diff %t.tgt2 %t.res.tgt2 +// RUN: clang-offload-bundler -type=ast -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.ast -unbundle +// RUN: diff %t.tgt1 %t.res.tgt1 // Check if we can unbundle a file with no magic strings. // RUN: clang-offload-bundler -type=bc
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66601/new/ https://reviews.llvm.org/D66601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev marked an inline comment as done.
sdmitriev added inline comments.
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;
ABataev wrote:
> ABataev wrote:
> > 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?
> What about this?
Based on the comments on lines 775-776 and 801 in ClangOffloadBundler.cpp
bundler will create empty files for missing device bundles.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
ABataev added inline comments.
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;
ABataev wrote:
> 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?
What about this?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev updated this revision to Diff 217522.
sdmitriev marked an inline comment as done.
sdmitriev added a comment.
Added tests for each file type.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -791,8 +791,9 @@
return false;
}
- // If we found elements, we emit an error if none of those were for the host.
- if (!FoundHostBundle) {
+ // If we found elements, we emit an error if none of those were for the host
+ // 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";
return true;
}
@@ -895,7 +896,9 @@
++Index;
}
- if (HostTargetNum != 1) {
+ // Host triple is not really needed for unbundling operation, so do not
+ // treat missing host triple as error if we do unbundling.
+ if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
Error = true;
errs() << "error: expecting exactly one host target but got "
<< HostTargetNum << ".\n";
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -156,14 +156,20 @@
// RUN: diff %t.i %t.res.i
// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=i -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.i -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: clang-offload-bundler -type=ii -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ii,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ii -unbundle
// RUN: diff %t.ii %t.res.ii
// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=ii -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.ii -unbundle
+// RUN: diff %t.tgt2 %t.res.tgt2
// RUN: clang-offload-bundler -type=ll -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ll,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ll -unbundle
// RUN: diff %t.ll %t.res.ll
// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=ll -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.ll -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: clang-offload-bundler -type=s -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.s,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.s -unbundle
// RUN: diff %t.s %t.res.s
// RUN: diff %t.tgt1 %t.res.tgt1
@@ -172,6 +178,8 @@
// RUN: diff %t.s %t.res.s
// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=s -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt2 %t.res.tgt2
// Check if we can unbundle a file with no magic strings.
// RUN: clang-offload-bundler -type=s -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.s,%t.res.tgt1,%t.res.tgt2 -inputs=%t.s -unbundle
@@ -183,6 +191,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that bindler prints an error if given host bundle does not exist in the fat binary.
+// RUN: not clang-offload-bundler -type=s -targets=host-x86_64-xxx-linux-gnu,openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.s,%t.res.tgt1 -inputs=%t.bundle3.s -unbundle 2>&1 | FileCheck %s --check-prefix CK-NO-HOST-BUNDLE
+// CK-NO-HOST-BUNDLE: error: Can't find bundle for the host target
+
//
// Check binary bundle/unbundle. The content that we have before bundling must be the same we have after unbundling.
//
@@ -194,10 +206,14 @@
// RUN: diff %t.bc %t.res.bc
// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=bc -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.bc -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: clang-offload-bundler -type=gch -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.gch,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.gch -unbundle
// RUN: diff %t.ast %t.res.gch
// RUN: diff %t.tgt1 %t.res.tgt1
// RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=gch -targets=openmp-x86_64-pc-linux-gnu
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev marked an inline comment as done. sdmitriev 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 + sdmitriev wrote: > ABataev wrote: > > What about tests for other kinds of elements like preprocessed code, IR, > > objects, etc.? > Ok, I can add more tests for reprocessed code, IR, etc. I have added one test > per file handler type so far, but I can do it per file type. BTW, test for > object type is already there on line 264. One note that adding more tests for binary file handler would most probably require this fix https://reviews.llvm.org/D66598. Can you please take a look at this patch? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66601/new/ https://reviews.llvm.org/D66601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev marked an inline comment as done. sdmitriev 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 + ABataev wrote: > What about tests for other kinds of elements like preprocessed code, IR, > objects, etc.? Ok, I can add more tests for reprocessed code, IR, etc. I have added one test per file handler type so far, but I can do it per file type. BTW, test for object type is already there on line 264. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66601/new/ https://reviews.llvm.org/D66601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev marked an inline comment as done.
sdmitriev added inline comments.
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:
> > 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?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev marked an inline comment as done.
sdmitriev added inline comments.
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:
> 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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev updated this revision to Diff 217259.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -790,8 +790,9 @@
return false;
}
- // If we found elements, we emit an error if none of those were for the host.
- if (!FoundHostBundle) {
+ // If we found elements, we emit an error if none of those were for the host
+ // 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";
return true;
}
@@ -894,7 +895,9 @@
++Index;
}
- if (HostTargetNum != 1) {
+ // Host triple is not really needed for unbundling operation, so do not
+ // treat missing host triple as error if we do unbundling.
+ if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
Error = true;
errs() << "error: expecting exactly one host target but got "
<< HostTargetNum << ".\n";
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -183,6 +183,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=s
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1
-inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
//
// Check binary bundle/unbundle. The content that we have before bundling must
be the same we have after unbundling.
//
@@ -221,6 +225,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// 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
+
//
// Check object bundle/unbundle. The content should be bundled into an ELF
// section (we are using a PowerPC little-endian host which uses ELF). We
@@ -253,6 +261,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=o
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1
-inputs=%t.bundle3.o -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
// Some code so that we can create a binary out of this file.
int A = 0;
void test_func(void) {
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -790,8 +790,9 @@
return false;
}
- // If we found elements, we emit an error if none of those were for the host.
- if (!FoundHostBundle) {
+ // If we found elements, we emit an error if none of those were for the host
+ // 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";
return true;
}
@@ -894,7 +895,9 @@
++Index;
}
- if (HostTargetNum != 1) {
+ // Host triple is not really needed for unbundling operation, so do not
+ // treat missing host triple as error if we do unbundling.
+ if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
Error = true;
errs() << "error: expecting exactly one host target but got "
<< HostTargetNum << ".\n";
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -183,6 +183,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=s -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
//
// Check binary bundle/unbundle. The content that we have before bundling must be the same we have after unbundling.
//
@@ -221,6 +225,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=ast
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev marked an inline comment as done.
sdmitriev added inline comments.
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;
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?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
ABataev added inline comments.
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:
> 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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev marked an inline comment as done.
sdmitriev added inline comments.
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;
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
ABataev added inline comments.
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;
I believe, for unbundling we also must check for `!= 1` rather than `> 1`.
Zero host targets also is not allowed.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev updated this revision to Diff 217234.
sdmitriev added a comment.
Rebase.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66601/new/
https://reviews.llvm.org/D66601
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -739,7 +739,6 @@
// Read all the bundles that are in the work list. If we find no bundles we
// assume the file is meant for the host target.
- bool FoundHostBundle = false;
while (!Worklist.empty()) {
StringRef CurTriple = FH.get()->ReadBundleStart(Input);
@@ -765,10 +764,6 @@
FH.get()->ReadBundle(OutputFile, Input);
FH.get()->ReadBundleEnd(Input);
Worklist.erase(Output);
-
-// Record if we found the host bundle.
-if (hasHostKind(CurTriple))
- FoundHostBundle = true;
}
// If no bundles were found, assume the input file is the host bundle and
@@ -790,12 +785,6 @@
return false;
}
- // If we found elements, we emit an error if none of those were for the host.
- if (!FoundHostBundle) {
-errs() << "error: Can't find bundle for the host target\n";
-return true;
- }
-
// If we still have any elements in the worklist, create empty files for
them.
for (auto : Worklist) {
std::error_code EC;
@@ -894,7 +883,9 @@
++Index;
}
- if (HostTargetNum != 1) {
+ // Host triple is not really needed for unbundling operation, so do not
+ // treat missing host triple as error if we do unbundling.
+ if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
Error = true;
errs() << "error: expecting exactly one host target but got "
<< HostTargetNum << ".\n";
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -183,6 +183,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=s
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1
-inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
//
// Check binary bundle/unbundle. The content that we have before bundling must
be the same we have after unbundling.
//
@@ -221,6 +225,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// 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
+
//
// Check object bundle/unbundle. The content should be bundled into an ELF
// section (we are using a PowerPC little-endian host which uses ELF). We
@@ -253,6 +261,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=o
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1
-inputs=%t.bundle3.o -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
// Some code so that we can create a binary out of this file.
int A = 0;
void test_func(void) {
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -739,7 +739,6 @@
// Read all the bundles that are in the work list. If we find no bundles we
// assume the file is meant for the host target.
- bool FoundHostBundle = false;
while (!Worklist.empty()) {
StringRef CurTriple = FH.get()->ReadBundleStart(Input);
@@ -765,10 +764,6 @@
FH.get()->ReadBundle(OutputFile, Input);
FH.get()->ReadBundleEnd(Input);
Worklist.erase(Output);
-
-// Record if we found the host bundle.
-if (hasHostKind(CurTriple))
- FoundHostBundle = true;
}
// If no bundles were found, assume the input file is the host bundle and
@@ -790,12 +785,6 @@
return false;
}
- // If we found elements, we emit an error if none of those were for the host.
- if (!FoundHostBundle) {
-errs() << "error: Can't find bundle for the host target\n";
-return true;
- }
-
// If we still have any elements in the worklist, create empty files for them.
for (auto : Worklist) {
std::error_code EC;
@@ -894,7 +883,9 @@
++Index;
}
- if (HostTargetNum != 1) {
+ // Host triple is not really needed for unbundling operation, so do not
+ // treat missing host triple as
[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles
sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
sdmitriev added a reviewer: hfinkel.
Bundler currently requires host triple to be provided no matter if you are
performing bundling or unbundling, but for unbundling operation such
requirement is too restrictive. You may for example want to examine device part
of the object for a particular offload target, but you have to extract host
part as well even though you do not need it. Host triple isn't really needed
for unbundling, so this patch removes that requirement.
Repository:
rC Clang
https://reviews.llvm.org/D66601
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -833,7 +833,6 @@
// Read all the bundles that are in the work list. If we find no bundles we
// assume the file is meant for the host target.
- bool FoundHostBundle = false;
while (!Worklist.empty()) {
StringRef CurTriple = FH.get()->ReadBundleStart(Input);
@@ -859,10 +858,6 @@
FH.get()->ReadBundle(OutputFile, Input);
FH.get()->ReadBundleEnd(Input);
Worklist.erase(Output);
-
-// Record if we found the host bundle.
-if (hasHostKind(CurTriple))
- FoundHostBundle = true;
}
// If no bundles were found, assume the input file is the host bundle and
@@ -884,12 +879,6 @@
return false;
}
- // If we found elements, we emit an error if none of those were for the host.
- if (!FoundHostBundle) {
-errs() << "error: Can't find bundle for the host target\n";
-return true;
- }
-
// If we still have any elements in the worklist, create empty files for
them.
for (auto : Worklist) {
std::error_code EC;
@@ -988,7 +977,9 @@
++Index;
}
- if (HostTargetNum != 1) {
+ // Host triple is not really needed for unbundling operation, so do not
+ // treat missing host triple as error if we do unbundling.
+ if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
Error = true;
errs() << "error: expecting exactly one host target but got "
<< HostTargetNum << ".\n";
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -183,6 +183,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=s
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1
-inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
//
// Check binary bundle/unbundle. The content that we have before bundling must
be the same we have after unbundling.
//
@@ -221,6 +225,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// 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
+
//
// Check object bundle/unbundle. The content should be bundled into an ELF
// section (we are using a PowerPC little-endian host which uses ELF). We
@@ -256,6 +264,10 @@
// RUN: diff %t.empty %t.res.tgt1
// RUN: diff %t.empty %t.res.tgt2
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=o
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1
-inputs=%t.bundle3.o -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
// Some code so that we can create a binary out of this file.
int A = 0;
void test_func(void) {
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -833,7 +833,6 @@
// Read all the bundles that are in the work list. If we find no bundles we
// assume the file is meant for the host target.
- bool FoundHostBundle = false;
while (!Worklist.empty()) {
StringRef CurTriple = FH.get()->ReadBundleStart(Input);
@@ -859,10 +858,6 @@
FH.get()->ReadBundle(OutputFile, Input);
FH.get()->ReadBundleEnd(Input);
Worklist.erase(Output);
-
-// Record if we found the host bundle.
-if (hasHostKind(CurTriple))
- FoundHostBundle = true;
}
// If no bundles were found, assume the input file is the host bundle and
@@ -884,12 +879,6 @@
return
