[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[Sergey Dmitriev via Phabricator via cfe-commits]

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