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