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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8320
+        TC->getDriver().isUsingLTO(/* IsOffload */ true)
+            ? ",feature=" + llvm::join(FeatureArgs, ",feature=")
+            : "";
----------------
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.




================
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));
----------------
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)

```


================
Comment at: clang/test/Driver/openmp-offload-gpu-new.c:117
+
+// CHECK-LTO-FEATURES: clang-offload-packager{{.*}}feature={{.*}}ptx
----------------
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).


================
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;
----------------
This looks like a homegrown version of `getLastArg()`. I wonder if there's a way to use the standard LLVM option parser for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127686