[PATCH] D127686: [Offloading] Embed the target features in the OffloadBinary

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

Thanks for the comments, I'll try to address them.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8320
+        TC->getDriver().isUsingLTO(/* IsOffload */ true)
+            ? ",feature=" + llvm::join(FeatureArgs, ",feature=")
+            : "";
----------------
tra wrote:
> This makes a couple of implicit assumptions that we should probably make more explicit.
> - `assert(!FeatureArgs.empty())`, otherwise we may end up passing an empty `feature=`.
> -  that it will be appended to the comma-separated list, which is not obvious until we actually do it later.
> 
> 
I think I should add some appropriate logic to not add this string and empty comma if the features are empty. I'll try to include that in a cleanup here. 


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8323-8327
     CmdArgs.push_back(Args.MakeArgString(
         "--image=file=" + File + "," + "triple=" + TC->getTripleString() + "," +
         "arch=" + Arch + "," + "kind=" +
-        Action::GetOffloadKindName(OffloadAction->getOffloadingDeviceKind())));
+        Action::GetOffloadKindName(OffloadAction->getOffloadingDeviceKind()) +
+        FeaturesStr));
----------------
tra wrote:
> I think this would look cleaner if expressed along these lines:
> ```
> 
> SmallVector<StringRef> Parts = {
>   "--image=file=" + File,
>   "triple=" + TC->getTripleString() ,
>   "arch=" + Arch,
>   "kind=" + Action::GetOffloadKindName(OffloadAction->getOffloadingDeviceKind()),
> };
> 
> // Add "-feature=foo" arguments to `Parts` here.
> 
> llvm::join(",", Parts)
> 
> ```
Yeah, it would probably be best to organize something like this. Though we would either need to make them strings, use a string saver, or store a Twine (which probably isn't recommended). I'll try to clean it up like this.


================
Comment at: clang/test/Driver/openmp-offload-gpu-new.c:117
+
+// CHECK-LTO-FEATURES: clang-offload-packager{{.*}}feature={{.*}}ptx
----------------
tra wrote:
> This should probably be a bit more specific/verbose. E.g. I'd want to make sure that `feature=` is part of the `--image` argument and that ptx belongs to it and is not part of some other argument (or even a file name extension).
Sure, I was just hesitant to make it super specific since the specific feature will change depending on the CUDA installation (or lack thereof).


================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:81-85
+      if (Args.count(KeyAndValue.first))
+        Args[KeyAndValue.first] =
+            Saver.save(Args[KeyAndValue.first] + "," + KeyAndValue.second);
+      else
+        Args[KeyAndValue.first] = KeyAndValue.second;
----------------
tra wrote:
> This looks like a homegrown version of `getLastArg()`. I wonder if there's a way to use the standard LLVM option parser for that.
It's copy-pasted from `unifyTargetFeatures` in Clang, which does the same thing. I may be able to simplify this, but for now I just wanted to copy something that ostensibly worked. Using LLVM option parsing stuff is another thing on my list. I want to pass arguments to the linker wrapper via some `--offload-opt` similar to `--plugin-opt` for LTO. Since my ultimate goal is to incorporate this into a linker completely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127686