[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Wrapper tool is invoked at link phase, therefore there just could be no .bc 
files available to read data layout from. Ok, looks like there is no good 
solution for the data layout problem, so I will drop the idea of passing 
wrapper .bc directly to the linker when LTO is enabled (at least for now:). I 
will abandon this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D70551#1757065 , @sdmitriev wrote:

> I would agree with you if data layout was indeed available in the driver, but 
> unfortunately driver does not have access to any existing TargetInfo instance 
> (or maybe I just have not found it). So, I have to create TargetInfo and 
> build data layout even for the case when driver passes it to the wrapper 
> tool. I guess that would also be a default data layout, so it would not 
> differ much from what I have already done in this patch. BTW, I will probably 
> upload an alternative patch where I have implemented your suggestion, just to 
> compare))


Of course, this is not a good solution. I was hoping that TargetInfo is 
available in driver. Otherwise, it would be good somehow to read the target 
data from the .bc file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

I would agree with you if data layout was indeed available in the driver, but 
unfortunately driver does not have access to any existing TargetInfo instance 
(or maybe I just have not found it). So, I have to create TargetInfo and build 
data layout even for the case when driver passes it to the wrapper tool. I 
guess that would also be a default data layout, so it would not differ much 
from what I have already done in this patch. BTW, I will probably upload an 
alternative patch where I have implemented your suggestion, just to compare))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D70551#1756997 , @sdmitriev wrote:

> Ok, it is possible to do it like you suggested, but I think that teaching 
> wrapper tool to set data layout without external help is more preferable. 
> There is a certain difference between opt and wrapper tool – opt works on the 
> existing .bc that is provided in command line and data-layout option just 
> gives user an optional way to override input’s data layout while wrapper tool 
> creates output .bc from scratch. With your proposal, data-layout would become 
> sort of mandatory option for the wrapper tool which is not very convenient. I 
> believe wrapper tool should be able to set it without external help, and we 
> can always add an option to override data layout (similar to opt) if there 
> would be a need for that.


Why it is not convenient? Plus, when you're trying to build the data layout 
yourself, you end up with the default one for the given target rather than rely 
on the one specified by the user when the driver was invoked. So, I think, it 
would be better to get the data layout from the driver rather than use the 
default one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Ok, it is possible to do it like you suggested, but I think that teaching 
wrapper tool to set data layout without external help is more preferable. There 
is a certain difference between opt and wrapper tool – opt works on the 
existing .bc that is provided in command line and data-layout option just gives 
user an optional way to override input’s data layout while wrapper tool creates 
output .bc from scratch. With your proposal, data-layout would become sort of 
mandatory option for the wrapper tool which is not very convenient. I believe 
wrapper tool should be able to set it without external help, and we can always 
add an option to override data layout (similar to opt) if there would be a need 
for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D70551#1755768 , @ABataev wrote:

> In D70551#1755761 , @sdmitriev wrote:
>
> > In D70551#1755583 , @ABataev wrote:
> >
> > > In D70551#173 , @sdmitriev 
> > > wrote:
> > >
> > > > In D70551#1755527 , @ABataev 
> > > > wrote:
> > > >
> > > > > Why do we need this?
> > > >
> > > >
> > > > To pass wrapper bitcode directly to the linker when LTO is enabled. LTO 
> > > > plugin does not accept bc if there is no data layout.
> > >
> > >
> > > Maybe it would better to pass it a parameter just like for opt?
> >
> >
> > In this case clang driver would need to get it from somewhere in order to 
> > pass it to the wrapper tool. I do not know where it can get it from.
>
>
> `TargetInfo` class is not accessible in the driver?


Ah, I see, you mean clang's TargetInfo. Let me check if/how it will work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D70551#1755761 , @sdmitriev wrote:

> In D70551#1755583 , @ABataev wrote:
>
> > In D70551#173 , @sdmitriev 
> > wrote:
> >
> > > In D70551#1755527 , @ABataev 
> > > wrote:
> > >
> > > > Why do we need this?
> > >
> > >
> > > To pass wrapper bitcode directly to the linker when LTO is enabled. LTO 
> > > plugin does not accept bc if there is no data layout.
> >
> >
> > Maybe it would better to pass it a parameter just like for opt?
>
>
> In this case clang driver would need to get it from somewhere in order to 
> pass it to the wrapper tool. I do not know where it can get it from.


`TargetInfo` class is not accessible in the driver?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D70551#1755583 , @ABataev wrote:

> In D70551#173 , @sdmitriev wrote:
>
> > In D70551#1755527 , @ABataev wrote:
> >
> > > Why do we need this?
> >
> >
> > To pass wrapper bitcode directly to the linker when LTO is enabled. LTO 
> > plugin does not accept bc if there is no data layout.
>
>
> Maybe it would better to pass it a parameter just like for opt?


In this case clang driver would need to get it from somewhere in order to pass 
it to the wrapper tool. I do not know where it can get it from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D70551#173 , @sdmitriev wrote:

> In D70551#1755527 , @ABataev wrote:
>
> > Why do we need this?
>
>
> To pass wrapper bitcode directly to the linker when LTO is enabled. LTO 
> plugin does not accept bc if there is no data layout.


Maybe it would better to pass it a parameter just like for opt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D70551#1755527 , @ABataev wrote:

> Why do we need this?


To pass wrapper bitcode directly to the linker when LTO is enabled. LTO plugin 
does not accept bc if there is no data layout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Why do we need this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70551: [clang-offload-wrapper] Add data layout to the wrapper bitcode

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70551

Files:
  clang/test/Driver/clang-offload-wrapper.c
  clang/tools/clang-offload-wrapper/CMakeLists.txt
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp

Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -29,9 +29,13 @@
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Target/TargetMachine.h"
+#include "llvm/Target/TargetOptions.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 #include 
 #include 
@@ -55,9 +59,9 @@
 cl::cat(ClangOffloadWrapperCategory));
 
 static cl::opt
-Target("target", cl::Required,
-   cl::desc("Target triple for the output module"),
-   cl::value_desc("triple"), cl::cat(ClangOffloadWrapperCategory));
+TargetTriple("target", cl::Required,
+ cl::desc("Target triple for the output module"),
+ cl::value_desc("triple"), cl::cat(ClangOffloadWrapperCategory));
 
 namespace {
 
@@ -287,8 +291,9 @@
   }
 
 public:
-  BinaryWrapper(StringRef Target) : M("offload.wrapper.object", C) {
-M.setTargetTriple(Target);
+  BinaryWrapper(const TargetMachine *TM) : M("offload.wrapper.object", C) {
+M.setTargetTriple(TM->getTargetTriple().getTriple());
+M.setDataLayout(TM->createDataLayout());
   }
 
   const Module (ArrayRef> Binaries) {
@@ -321,16 +326,33 @@
 return 0;
   }
 
+  InitializeAllTargets();
+  InitializeAllTargetMCs();
+
   auto reportError = [argv](Error E) {
 logAllUnhandledErrors(std::move(E), WithColor::error(errs(), argv[0]));
   };
 
-  if (Triple(Target).getArch() == Triple::UnknownArch) {
-reportError(createStringError(
-errc::invalid_argument, "'" + Target + "': unsupported target triple"));
+  if (Triple(TargetTriple).getArch() == Triple::UnknownArch) {
+reportError(
+createStringError(errc::invalid_argument,
+  "'" + TargetTriple + "': unsupported target triple"));
 return 1;
   }
 
+  // Lookup and create target machine. We need it for setting data layout for
+  // the wrapper module.
+  std::string Error;
+  const Target *TheTarget = TargetRegistry::lookupTarget(TargetTriple, Error);
+  if (!TheTarget) {
+reportError(createStringError(errc::invalid_argument, Error));
+return 1;
+  }
+
+  std::unique_ptr TM(TheTarget->createTargetMachine(
+  TargetTriple, "", "", TargetOptions(), None));
+  assert(TM && "Could not allocate target machine!");
+
   // Read device binaries.
   SmallVector, 4u> Buffers;
   SmallVector, 4u> Images;
@@ -357,7 +379,7 @@
   }
 
   // Create a wrapper for device binaries and write its bitcode to the file.
-  WriteBitcodeToFile(BinaryWrapper(Target).wrapBinaries(
+  WriteBitcodeToFile(BinaryWrapper(TM.get()).wrapBinaries(
  makeArrayRef(Images.data(), Images.size())),
  Out.os());
   if (Out.os().has_error()) {
Index: clang/tools/clang-offload-wrapper/CMakeLists.txt
===
--- clang/tools/clang-offload-wrapper/CMakeLists.txt
+++ clang/tools/clang-offload-wrapper/CMakeLists.txt
@@ -1,4 +1,15 @@
-set(LLVM_LINK_COMPONENTS BitWriter Core Support TransformUtils)
+set(LLVM_LINK_COMPONENTS
+  AllTargetsAsmParsers
+  AllTargetsCodeGens
+  AllTargetsDescs
+  AllTargetsInfos
+  BitWriter
+  Core
+  MC
+  Support
+  Target
+  TransformUtils
+  )
 
 if(NOT CLANG_BUILT_STANDALONE)
   set(tablegen_deps intrinsics_gen)
Index: clang/test/Driver/clang-offload-wrapper.c
===
--- clang/test/Driver/clang-offload-wrapper.c
+++ clang/test/Driver/clang-offload-wrapper.c
@@ -22,6 +22,7 @@
 // RUN: clang-offload-wrapper -target=x86_64-pc-linux-gnu -o %t.wrapper.bc %t.tgt
 // RUN: llvm-dis %t.wrapper.bc -o - | FileCheck %s --check-prefix CHECK-IR
 
+// CHECK-IR: target datalayout =
 // CHECK-IR: target triple = "x86_64-pc-linux-gnu"
 
 // CHECK-IR-DAG: [[ENTTY:%.+]] = type { i8*, i8*, i{{32|64}}, i32, i32 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits