[PATCH] D133705: [HIP] Fix unbundling archive

[Artem Belevich via Phabricator via cfe-commits]

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