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

Reply via email to