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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
