tra added a comment.
It's still not quite clear to me what the patch is intended to do. The
description describes the existing issues (we don't unbundle some libraries),
but is not clear what we do want to have in the end.
Are all library arguments expected to be unbundled? If we do not want to
unbundle some libraries, then which arguments would lead to that outcome. The
tests only seem to verify the cases where unbundling does happen. It would be
great to test the cases when it should not. That is, assuming that there are
such cases -- the code seems to
================
Comment at: clang/lib/Driver/Driver.cpp:2907-2908
+ // which are not object files. Files with extension ".lib" is
classified
+ // as TY_Object but they are actually archives, therefore should not be
+ // unbundled.
+ const StringRef LibFileExt = ".lib";
----------------
This is confusing. I do not understand how/why `therefore should not be
unbundled` is inferred from `they are actually archives`.
The patch description says that not unbundling the archives is that the patch
is intended to fix. The tests below with `MSVC` label show that we do unbundle
the inputs with `.lib` extension.
Should this comment be fixed/updated? What do I miss?
================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1828-1838
+ for (auto Prefix : {"/libdevice/", "/"}) {
+ if (Lib.startswith(":"))
+ AOBFileNames.push_back(
+ Twine(LPath + Prefix + Lib.drop_front()).str());
+ else if (IsMSVC)
+ AOBFileNames.push_back(Twine(LPath + Prefix + Lib + Ext).str());
+ else
----------------
Nit: I'd restructure it a bit and move library file name construction out of
the loop. Makes it a bit easier to see what's going on.
```
auto LibFile = Lib.startswith(":") ? Lib.drop_front() :
IsMSVC ? Lib+Ext : "lib" + Lib + Ext;
for (auto Prefix : {"/libdevice/", "/"})
AOBFileNames.push_back( Twine(LPath + Prefix + LibFile).str());
```
You may also skip `AOBFileNames` altogether and just check for the file
existence directly within that for loop.
================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1833
+ else if (IsMSVC)
+ AOBFileNames.push_back(Twine(LPath + Prefix + Lib + Ext).str());
+ else
----------------
Is it intentional that we're not adding `lib` prefix to library names passed
with `-l`?
================
Comment at: clang/test/Driver/hip-link-bundle-archive.hip:45
+
+// GNU1: "{{.*}}clang-offload-bundler" "-unbundle" "-type=a"
"-input={{.*}}[[LIB:libhipBundled\.a]]"
"-targets=hip-amdgcn-amd-amdhsa-gfx1030" "-output=[[A1030:.*\.a]]"
"-allow-missing-bundles"
+// GNU2: "{{.*}}clang-offload-bundler" "-unbundle" "-type=a"
"-input={{.*}}[[LIB:libhipBundled\.a\.5\.2]]"
"-targets=hip-amdgcn-amd-amdhsa-gfx1030" "-output=[[A1030:.*\.a]]"
"-allow-missing-bundles"
----------------
Just curious. Does `GNU` have some meaning in the check label?
`GNU[12]` seem to deal with unbundling while `GNU-L*` seem to be regular host
libraries. It would be useful to use somewhat more descriptive labels. E.g.
`HOST*`/`OFFLOAD*` ?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133705/new/
https://reviews.llvm.org/D133705
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
