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

[Joseph Huber via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6e6889288cdc: [Offloading] Embed the target features in the 
OffloadBinary (authored by jhuber6).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127686

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/openmp-offload-gpu-new.c
  clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
  clang/tools/clang-offload-packager/ClangOffloadPackager.cpp

Index: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
===
--- clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
+++ clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
@@ -22,6 +22,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/WithColor.h"
 
 using namespace llvm;
@@ -71,9 +72,18 @@
   SmallVector BinaryData;
   raw_svector_ostream OS(BinaryData);
   for (StringRef Image : DeviceImages) {
+BumpPtrAllocator Alloc;
+StringSaver Saver(Alloc);
+
 StringMap Args;
-for (StringRef Arg : llvm::split(Image, ","))
-  Args.insert(Arg.split("="));
+for (StringRef Arg : llvm::split(Image, ",")) {
+  auto KeyAndValue = Arg.split("=");
+  if (Args.count(KeyAndValue.first))
+Args[KeyAndValue.first] =
+Saver.save(Args[KeyAndValue.first] + "," + KeyAndValue.second);
+  else
+Args[KeyAndValue.first] = KeyAndValue.second;
+}
 
 if (!Args.count("triple") || !Args.count("file"))
   return reportError(createStringError(
Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -68,10 +68,6 @@
cl::desc("Path of linker binary"),
cl::cat(ClangLinkerWrapperCategory));
 
-static cl::opt
-TargetFeatures("target-feature", cl::desc("Target features for triple"),
-   cl::cat(ClangLinkerWrapperCategory));
-
 static cl::opt OptLevel("opt-level",
  cl::desc("Optimization level for LTO"),
  cl::init("O2"),
@@ -726,16 +722,24 @@
   }
 }
 
-// Get the target features passed in from the driver as =.
-std::vector getTargetFeatures(const Triple &TheTriple) {
-  std::vector Features;
-  auto TargetAndFeatures = StringRef(TargetFeatures).split('=');
-  if (TargetAndFeatures.first != TheTriple.getTriple())
-return Features;
+// Get the list of target features from the input file and unify them such that
+// if there are multiple +xxx or -xxx features we only keep the last one.
+std::vector getTargetFeatures(ArrayRef InputFiles) {
+  SmallVector Features;
+  for (const OffloadFile &File : InputFiles) {
+for (auto Arg : llvm::split(File.getBinary()->getString("feature"), ","))
+  Features.emplace_back(Arg);
+  }
+
+  // Only add a feature if it hasn't been seen before starting from the end.
+  std::vector UnifiedFeatures;
+  DenseSet UsedFeatures;
+  for (StringRef Feature : llvm::reverse(Features)) {
+if (UsedFeatures.insert(Feature.drop_front()).second)
+  UnifiedFeatures.push_back(Feature.str());
+  }
 
-  for (auto Feature : llvm::split(TargetAndFeatures.second, ','))
-Features.push_back(Feature.str());
-  return Features;
+  return UnifiedFeatures;
 }
 
 CodeGenOpt::Level getCGOptLevel(unsigned OptLevel) {
@@ -755,6 +759,7 @@
 template >
 std::unique_ptr createLTO(
 const Triple &TheTriple, StringRef Arch, bool WholeProgram,
+const std::vector &Features,
 ModuleHook Hook = [](size_t, const Module &) { return true; }) {
   lto::Config Conf;
   lto::ThinBackend Backend;
@@ -765,7 +770,7 @@
   Conf.CPU = Arch.str();
   Conf.Options = codegen::InitTargetOptionsFromCodeGenFlags(TheTriple);
 
-  Conf.MAttrs = getTargetFeatures(TheTriple);
+  Conf.MAttrs = Features;
   Conf.CGOptLevel = getCGOptLevel(OptLevel[1] - '0');
   Conf.OptLevel = OptLevel[1] - '0';
   if (Conf.OptLevel > 0)
@@ -902,10 +907,12 @@
   };
 
   // We assume visibility of the whole program if every input file was bitcode.
+  auto Features = getTargetFeatures(BitcodeInputFiles);
   bool WholeProgram = InputFiles.empty();
   auto LTOBackend =
-  (EmbedBitcode) ? createLTO(TheTriple, Arch, WholeProgram, OutputBitcode)
- : createLTO(TheTriple, Arch, WholeProgram);
+  (EmbedBitcode)
+  ? createLTO(TheTriple, Arch, WholeProgram, Features, OutputBitcode)
+  : createLTO(TheTriple, Arch, WholeProgram, Features);
 
   // We need to resolve the symbols so the LTO 

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

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

LGTM with a nit.




Comment at: clang/test/Driver/openmp-offload-gpu-new.c:117
+
+// CHECK-LTO-FEATURES: clang-offload-packager{{.*}}feature={{.*}}ptx

tra wrote:
> jhuber6 wrote:
> > 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).
> I'm all for concise tests as long as they:
> 
> - express what you want to verify in a way that the reader would be able to 
> understand w/o having to look at the source code. 
> - are reasonably robust and do not produce false positives. `.*` wildcards 
> make it very easy to match things unintentionally.  Their use should be 
> carefully restricted. 
> 
> I wish FileCheck would have some sort of nested match check. E.g.
> 
> ```
> ; CHECK : [[CANDIDATE:--option.*?\s+]]
> ; CHECK:  [[SUBMATCH1: match something within [[CANDIDATE
> ; CHECK:  [[SUBMATCH2: match something within [[SUBMATCH1
> ```
Nit: ^^^ looks like this one slipped through the cracks and wasn't addressed.


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

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

ping.


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

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 updated this revision to Diff 436611.
jhuber6 added a comment.

Does this approach work? I'm just using the reverse iterator and only adding 
the argument if it hasn't been seen yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127686

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/openmp-offload-gpu-new.c
  clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
  clang/tools/clang-offload-packager/ClangOffloadPackager.cpp

Index: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
===
--- clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
+++ clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
@@ -22,6 +22,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/WithColor.h"
 
 using namespace llvm;
@@ -71,9 +72,18 @@
   SmallVector BinaryData;
   raw_svector_ostream OS(BinaryData);
   for (StringRef Image : DeviceImages) {
+BumpPtrAllocator Alloc;
+StringSaver Saver(Alloc);
+
 StringMap Args;
-for (StringRef Arg : llvm::split(Image, ","))
-  Args.insert(Arg.split("="));
+for (StringRef Arg : llvm::split(Image, ",")) {
+  auto KeyAndValue = Arg.split("=");
+  if (Args.count(KeyAndValue.first))
+Args[KeyAndValue.first] =
+Saver.save(Args[KeyAndValue.first] + "," + KeyAndValue.second);
+  else
+Args[KeyAndValue.first] = KeyAndValue.second;
+}
 
 if (!Args.count("triple") || !Args.count("file"))
   return reportError(createStringError(
Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -68,10 +68,6 @@
cl::desc("Path of linker binary"),
cl::cat(ClangLinkerWrapperCategory));
 
-static cl::opt
-TargetFeatures("target-feature", cl::desc("Target features for triple"),
-   cl::cat(ClangLinkerWrapperCategory));
-
 static cl::opt OptLevel("opt-level",
  cl::desc("Optimization level for LTO"),
  cl::init("O2"),
@@ -726,16 +722,24 @@
   }
 }
 
-// Get the target features passed in from the driver as =.
-std::vector getTargetFeatures(const Triple &TheTriple) {
-  std::vector Features;
-  auto TargetAndFeatures = StringRef(TargetFeatures).split('=');
-  if (TargetAndFeatures.first != TheTriple.getTriple())
-return Features;
+// Get the list of target features from the input file and unify them such that
+// if there are multiple +xxx or -xxx features we only keep the last one.
+std::vector getTargetFeatures(ArrayRef InputFiles) {
+  SmallVector Features;
+  for (const OffloadFile &File : InputFiles) {
+for (auto Arg : llvm::split(File.getBinary()->getString("feature"), ","))
+  Features.emplace_back(Arg);
+  }
+
+  // Only add a feature if it hasn't been seen before starting from the end.
+  std::vector UnifiedFeatures;
+  DenseSet UsedFeatures;
+  for (StringRef Feature : llvm::reverse(Features)) {
+if (UsedFeatures.insert(Feature.drop_front()).second)
+  UnifiedFeatures.push_back(Feature.str());
+  }
 
-  for (auto Feature : llvm::split(TargetAndFeatures.second, ','))
-Features.push_back(Feature.str());
-  return Features;
+  return UnifiedFeatures;
 }
 
 CodeGenOpt::Level getCGOptLevel(unsigned OptLevel) {
@@ -755,6 +759,7 @@
 template >
 std::unique_ptr createLTO(
 const Triple &TheTriple, StringRef Arch, bool WholeProgram,
+const std::vector &Features,
 ModuleHook Hook = [](size_t, const Module &) { return true; }) {
   lto::Config Conf;
   lto::ThinBackend Backend;
@@ -765,7 +770,7 @@
   Conf.CPU = Arch.str();
   Conf.Options = codegen::InitTargetOptionsFromCodeGenFlags(TheTriple);
 
-  Conf.MAttrs = getTargetFeatures(TheTriple);
+  Conf.MAttrs = Features;
   Conf.CGOptLevel = getCGOptLevel(OptLevel[1] - '0');
   Conf.OptLevel = OptLevel[1] - '0';
   if (Conf.OptLevel > 0)
@@ -902,10 +907,12 @@
   };
 
   // We assume visibility of the whole program if every input file was bitcode.
+  auto Features = getTargetFeatures(BitcodeInputFiles);
   bool WholeProgram = InputFiles.empty();
   auto LTOBackend =
-  (EmbedBitcode) ? createLTO(TheTriple, Arch, WholeProgram, OutputBitcode)
- : createLTO(TheTriple, Arch, WholeProgram);
+  (EmbedBitcode)
+  ? createLTO(TheTriple, Arch, WholeProgram, Features, OutputBitcode)
+  : createLTO(TheTriple, Arch, WholeProgram, Features);
 
   // We need to resolve the symbols so the LTO backend knows which symbols need
   // to be kept or ca

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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:735
+  llvm::StringMap LastOpt;
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+StringRef Name = Features[I];

// Record the index of the last occurence of the feature.



Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:741
+
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+StringRef Name = Features[I];

// Populate UnifiedFeatures only with last mentions of specific feature.





Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:743-748
+llvm::StringMap::iterator LastI =
+LastOpt.find(Name.drop_front(1));
+assert(LastI != LastOpt.end());
+unsigned Last = LastI->second;
+if (Last != I)
+  continue;

```
auto FeatureName = Name.drop_front(1); 
if (!LastOpt.count(FeatureName)) // could be just 
assert(LastOpt.count(FeatureName))
  continue
if (LastOpt[FeatureName] != I) // could be merged with the `if` above
  continue;
```


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

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 updated this revision to Diff 436607.
jhuber6 added a comment.

Adjust how we generate arguments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127686

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/openmp-offload-gpu-new.c
  clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
  clang/tools/clang-offload-packager/ClangOffloadPackager.cpp

Index: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
===
--- clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
+++ clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
@@ -22,6 +22,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/WithColor.h"
 
 using namespace llvm;
@@ -71,9 +72,18 @@
   SmallVector BinaryData;
   raw_svector_ostream OS(BinaryData);
   for (StringRef Image : DeviceImages) {
+BumpPtrAllocator Alloc;
+StringSaver Saver(Alloc);
+
 StringMap Args;
-for (StringRef Arg : llvm::split(Image, ","))
-  Args.insert(Arg.split("="));
+for (StringRef Arg : llvm::split(Image, ",")) {
+  auto KeyAndValue = Arg.split("=");
+  if (Args.count(KeyAndValue.first))
+Args[KeyAndValue.first] =
+Saver.save(Args[KeyAndValue.first] + "," + KeyAndValue.second);
+  else
+Args[KeyAndValue.first] = KeyAndValue.second;
+}
 
 if (!Args.count("triple") || !Args.count("file"))
   return reportError(createStringError(
Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -68,10 +68,6 @@
cl::desc("Path of linker binary"),
cl::cat(ClangLinkerWrapperCategory));
 
-static cl::opt
-TargetFeatures("target-feature", cl::desc("Target features for triple"),
-   cl::cat(ClangLinkerWrapperCategory));
-
 static cl::opt OptLevel("opt-level",
  cl::desc("Optimization level for LTO"),
  cl::init("O2"),
@@ -726,16 +722,34 @@
   }
 }
 
-// Get the target features passed in from the driver as =.
-std::vector getTargetFeatures(const Triple &TheTriple) {
-  std::vector Features;
-  auto TargetAndFeatures = StringRef(TargetFeatures).split('=');
-  if (TargetAndFeatures.first != TheTriple.getTriple())
-return Features;
+// Get the list of target features from the input file and unify them such that
+// if there are multiple +xxx or -xxx features we only keep the last one.
+std::vector getTargetFeatures(ArrayRef InputFiles) {
+  SmallVector Features;
+  for (const OffloadFile &File : InputFiles)
+for (auto Arg : llvm::split(File.getBinary()->getString("feature"), ","))
+  Features.emplace_back(Arg);
+
+  std::vector UnifiedFeatures;
+  llvm::StringMap LastOpt;
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+StringRef Name = Features[I];
+assert(Name[0] == '-' || Name[0] == '+');
+LastOpt[Name.drop_front(1)] = I;
+  }
+
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+StringRef Name = Features[I];
+llvm::StringMap::iterator LastI =
+LastOpt.find(Name.drop_front(1));
+assert(LastI != LastOpt.end());
+unsigned Last = LastI->second;
+if (Last != I)
+  continue;
 
-  for (auto Feature : llvm::split(TargetAndFeatures.second, ','))
-Features.push_back(Feature.str());
-  return Features;
+UnifiedFeatures.push_back(Name.str());
+  }
+  return UnifiedFeatures;
 }
 
 CodeGenOpt::Level getCGOptLevel(unsigned OptLevel) {
@@ -755,6 +769,7 @@
 template >
 std::unique_ptr createLTO(
 const Triple &TheTriple, StringRef Arch, bool WholeProgram,
+const std::vector &Features,
 ModuleHook Hook = [](size_t, const Module &) { return true; }) {
   lto::Config Conf;
   lto::ThinBackend Backend;
@@ -765,7 +780,7 @@
   Conf.CPU = Arch.str();
   Conf.Options = codegen::InitTargetOptionsFromCodeGenFlags(TheTriple);
 
-  Conf.MAttrs = getTargetFeatures(TheTriple);
+  Conf.MAttrs = Features;
   Conf.CGOptLevel = getCGOptLevel(OptLevel[1] - '0');
   Conf.OptLevel = OptLevel[1] - '0';
   if (Conf.OptLevel > 0)
@@ -902,10 +917,12 @@
   };
 
   // We assume visibility of the whole program if every input file was bitcode.
+  auto Features = getTargetFeatures(BitcodeInputFiles);
   bool WholeProgram = InputFiles.empty();
   auto LTOBackend =
-  (EmbedBitcode) ? createLTO(TheTriple, Arch, WholeProgram, OutputBitcode)
- : createLTO(TheTriple, Arch, WholeProgram);
+  (EmbedBitcode)
+  ? createLTO(TheTriple, Arch, WholeProgram, Features, 

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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/test/Driver/openmp-offload-gpu-new.c:117
+
+// CHECK-LTO-FEATURES: clang-offload-packager{{.*}}feature={{.*}}ptx

jhuber6 wrote:
> 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).
I'm all for concise tests as long as they:

- express what you want to verify in a way that the reader would be able to 
understand w/o having to look at the source code. 
- are reasonably robust and do not produce false positives. `.*` wildcards make 
it very easy to match things unintentionally.  Their use should be carefully 
restricted. 

I wish FileCheck would have some sort of nested match check. E.g.

```
; CHECK : [[CANDIDATE:--option.*?\s+]]
; CHECK:  [[SUBMATCH1: match something within [[CANDIDATE
; CHECK:  [[SUBMATCH2: match something within [[SUBMATCH1
```


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

[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 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

[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 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

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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 created this revision.
jhuber6 added reviewers: jdoerfert, JonChesterfield, tianshilei1992, tra, 
yaxunl, saiislam.
Herald added a project: All.
jhuber6 requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1, MaskRay.
Herald added a project: clang.

The target features are necessary for correctly compiling most programs
in LTO mode. Currently, these are derived in clang at link time and
passed as an arguemnt to the linker wrapper. This is problematic because
it requires knowing the required toolchain at link time, which should
not be necessry. Instead, these features should be embedded into the
offloading binary so we can unify them in the linker wrapper for LTO.
This also required changing the offload packager to interpret multiple
arguments as concatenation with a comma. This is so we can still use the
`,` separator for the argument list.

Depends on D127246 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127686

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/openmp-offload-gpu-new.c
  clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
  clang/tools/clang-offload-packager/ClangOffloadPackager.cpp

Index: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
===
--- clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
+++ clang/tools/clang-offload-packager/ClangOffloadPackager.cpp
@@ -22,6 +22,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/WithColor.h"
 
 using namespace llvm;
@@ -71,9 +72,18 @@
   SmallVector BinaryData;
   raw_svector_ostream OS(BinaryData);
   for (StringRef Image : DeviceImages) {
+BumpPtrAllocator Alloc;
+StringSaver Saver(Alloc);
+
 StringMap Args;
-for (StringRef Arg : llvm::split(Image, ","))
-  Args.insert(Arg.split("="));
+for (StringRef Arg : llvm::split(Image, ",")) {
+  auto KeyAndValue = Arg.split("=");
+  if (Args.count(KeyAndValue.first))
+Args[KeyAndValue.first] =
+Saver.save(Args[KeyAndValue.first] + "," + KeyAndValue.second);
+  else
+Args[KeyAndValue.first] = KeyAndValue.second;
+}
 
 if (!Args.count("triple") || !Args.count("file"))
   return reportError(createStringError(
Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -68,10 +68,6 @@
cl::desc("Path of linker binary"),
cl::cat(ClangLinkerWrapperCategory));
 
-static cl::opt
-TargetFeatures("target-feature", cl::desc("Target features for triple"),
-   cl::cat(ClangLinkerWrapperCategory));
-
 static cl::opt OptLevel("opt-level",
  cl::desc("Optimization level for LTO"),
  cl::init("O2"),
@@ -726,16 +722,34 @@
   }
 }
 
-// Get the target features passed in from the driver as =.
-std::vector getTargetFeatures(const Triple &TheTriple) {
-  std::vector Features;
-  auto TargetAndFeatures = StringRef(TargetFeatures).split('=');
-  if (TargetAndFeatures.first != TheTriple.getTriple())
-return Features;
+// Get the list of target features from the input file and unify them such that
+// if there are multiple +xxx or -xxx features we only keep the last one.
+std::vector getTargetFeatures(ArrayRef InputFiles) {
+  SmallVector Features;
+  for (const OffloadFile &File : InputFiles)
+for (auto Arg : llvm::split(File.getBinary()->getString("feature"), ","))
+  Features.emplace_back(Arg);
+
+  std::vector UnifiedFeatures;
+  llvm::StringMap LastOpt;
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+StringRef Name = Features[I];
+assert(Name[0] == '-' || Name[0] == '+');
+LastOpt[Name.drop_front(1)] = I;
+  }
+
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+StringRef Name = Features[I];
+llvm::StringMap::iterator LastI =
+LastOpt.find(Name.drop_front(1));
+assert(LastI != LastOpt.end());
+unsigned Last = LastI->second;
+if (Last != I)
+  continue;
 
-  for (auto Feature : llvm::split(TargetAndFeatures.second, ','))
-Features.push_back(Feature.str());
-  return Features;
+UnifiedFeatures.push_back(Name.str());
+  }
+  return UnifiedFeatures;
 }
 
 CodeGenOpt::Level getCGOptLevel(unsigned OptLevel) {
@@ -755,6 +769,7 @@
 template >
 std::unique_ptr createLTO(
 const Triple &TheTriple, StringRef Arch, bool WholeProgram,
+const std::vector &Features,
 ModuleHook Hook = [](size_t, const Module &) { return true; }) {
   lto::Config Conf;
   lto::ThinBackend Backend;
@@ -765