[PATCH] D133705: [HIP] Fix unbundling archive

[Yaxun Liu via Phabricator via cfe-commits]

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