yaxunl marked 3 inline comments as done.
yaxunl added inline comments.

================
Comment at: clang/lib/Driver/OffloadBundler.cpp:1008
+    auto Output = Worklist.begin();
+    for (auto E = Worklist.end(); Output != E; Output++) {
+      if (isCodeObjectCompatible(
----------------
tra wrote:
> saiislam wrote:
> > tra wrote:
> > > The patch description implies that there are at least two classes of 
> > > compatible objects -- the ones that match exactly and the ones that are 
> > > not exact match, but are still compatible.
> > > 
> > > 
> > > Here we're iterating until we find the first compatible object. What if 
> > > we also have the object that matches exactly, but it's further down the 
> > > list. Is that a problem that we may pick one or the other, depending on 
> > > the order they happen to appear in the worklist? It would be good to add 
> > > a test case for this scenario.
> > Though it looks plausible, such a case is not possible.
> >  
> > From [[ 
> > https://clang.llvm.org/docs/ClangOffloadBundler.html#bundle-entry-id | 
> > Clang Offload Bundler's Documentation]]
> > > If there is an entry with a target feature specified as Any, then all 
> > > entries must specify that target feature as Any for the same processor.
> > 
> > 
> Does it mean that the bundler is supposed to error out if  I pass 
> `-targets=hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx906:xnack-` 
> ?
> 
> I've just tried it with a bundler built in recent LLVM tree and it accepts 
> such a mis of targets without complaining. 
> 
> 
Bundle entries in the same bundle should follow the constraints specified in 
the clang-offload-bundler documentation.

Currently, clang enforces the rule when specifying `--offload-arch` options, 
which is the most common approach to generate the clang-offload-bundler 
bundles. However, if users use clang-offload-bundler directly to generate the 
bundle, the rule is not enforced.

I will add the enforcement to clang-offload-bundler too.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134546/new/

https://reviews.llvm.org/D134546

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to