yaxunl marked 4 inline comments as done.
yaxunl added inline comments.
================
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";
----------------
tra wrote:
> 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?
Here the driver is trying to unbundle bundled objects. This is different from
unbundle archives. clang classifies '.lib' files as objects. If we do not
exclude '.lib' files here, they will be incorrectly unbundled as objects here.
The unbundling of '.lib' as archives should be done at other places.
Since this comment is confusing, I will change it to:
'therefore should not be unbundled as objects here. They will be handled at
other places.'
================
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
----------------
tra wrote:
> 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.
will do
================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1833
+ else if (IsMSVC)
+ AOBFileNames.push_back(Twine(LPath + Prefix + Lib + Ext).str());
+ else
----------------
tra wrote:
> Is it intentional that we're not adding `lib` prefix to library names passed
> with `-l`?
This is for MSVC, which assumes `-labc` corresponds to the lib file name
'abc.lib'.
================
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"
----------------
tra wrote:
> 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*` ?
Here 'GNU' is versus 'MSVC' regarding the library name ('.a' vs '.lib').
'GNU-L' is for '-l'. 'GNU-LA' is for '-l:*.a'. 'GNU-A' is for '*.a'.
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
