[PATCH] D117120: [Doc] Add documentation for the clang-offload-wrapper tool (NFC)

[Sergey Dmitriev via Phabricator via cfe-commits]

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

Looks good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117120

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

[PATCH] D114776: [clang-offload-bundler] Reuse original file extension for device archive member

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4b553297ef3e: [clang-offload-bundler] Reuse original file 
extension for device archive member (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114776

Files:
  clang/test/Driver/fat-archive-unbundle-ext.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -180,21 +180,19 @@
   }
 };
 
-static StringRef getDeviceFileExtension(StringRef Device) {
+static StringRef getDeviceFileExtension(StringRef Device,
+StringRef BundleFileName) {
   if (Device.contains("gfx"))
 return ".bc";
   if (Device.contains("sm_"))
 return ".cubin";
-
-  WithColor::warning() << "Could not determine extension for archive"
-  "members, using \".o\"\n";
-  return ".o";
+  return sys::path::extension(BundleFileName);
 }
 
 static std::string getDeviceLibraryFileName(StringRef BundleFileName,
 StringRef Device) {
   StringRef LibName = sys::path::stem(BundleFileName);
-  StringRef Extension = getDeviceFileExtension(Device);
+  StringRef Extension = getDeviceFileExtension(Device, BundleFileName);
 
   std::string Result;
   Result += LibName;
Index: clang/test/Driver/fat-archive-unbundle-ext.c
===
--- /dev/null
+++ clang/test/Driver/fat-archive-unbundle-ext.c
@@ -0,0 +1,21 @@
+// REQUIRES: x86-registered-target
+// UNSUPPORTED: windows, darwin, aix
+
+// Generate dummy fat object
+// RUN: %clang -O0 -target %itanium_abi_triple %s -c -o %t.host.o
+// RUN: echo 'Content of device file' > %t.tgt.o
+// RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-%itanium_abi_triple 
-inputs=%t.host.o,%t.tgt.o -outputs=%t.fat.obj
+
+// Then create a static archive with that object
+// RUN: rm -f %t.fat.a
+// RUN: llvm-ar cr %t.fat.a %t.fat.obj
+
+// Unbundle device part from the archive. Check that bundler does not print 
warnings.
+// RUN: clang-offload-bundler -unbundle -type=a 
-targets=openmp-%itanium_abi_triple -inputs=%t.fat.a -outputs=%t.tgt.a 2>&1 | 
FileCheck --allow-empty --check-prefix=CHECK-WARNING %s
+// CHECK-WARNING-NOT: warning
+
+// Check that device archive member inherited file extension from the original 
file.
+// RUN: llvm-ar t %t.tgt.a | FileCheck --check-prefix=CHECK-ARCHIVE %s
+// CHECK-ARCHIVE: {{\.obj$}}
+
+void foo(void) {}


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -180,21 +180,19 @@
   }
 };
 
-static StringRef getDeviceFileExtension(StringRef Device) {
+static StringRef getDeviceFileExtension(StringRef Device,
+StringRef BundleFileName) {
   if (Device.contains("gfx"))
 return ".bc";
   if (Device.contains("sm_"))
 return ".cubin";
-
-  WithColor::warning() << "Could not determine extension for archive"
-  "members, using \".o\"\n";
-  return ".o";
+  return sys::path::extension(BundleFileName);
 }
 
 static std::string getDeviceLibraryFileName(StringRef BundleFileName,
 StringRef Device) {
   StringRef LibName = sys::path::stem(BundleFileName);
-  StringRef Extension = getDeviceFileExtension(Device);
+  StringRef Extension = getDeviceFileExtension(Device, BundleFileName);
 
   std::string Result;
   Result += LibName;
Index: clang/test/Driver/fat-archive-unbundle-ext.c
===
--- /dev/null
+++ clang/test/Driver/fat-archive-unbundle-ext.c
@@ -0,0 +1,21 @@
+// REQUIRES: x86-registered-target
+// UNSUPPORTED: windows, darwin, aix
+
+// Generate dummy fat object
+// RUN: %clang -O0 -target %itanium_abi_triple %s -c -o %t.host.o
+// RUN: echo 'Content of device file' > %t.tgt.o
+// RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-%itanium_abi_triple -inputs=%t.host.o,%t.tgt.o -outputs=%t.fat.obj
+
+// Then create a static archive with that object
+// RUN: rm -f %t.fat.a
+// RUN: llvm-ar cr %t.fat.a %t.fat.obj
+
+// Unbundle device part from the archive. Check that bundler does not print warnings.
+// RUN: clang-offload-bundler -unbundle -type=a -targets=openmp-%itanium_abi_triple -inputs=%t.fat.a -outputs=%t.tgt.a 2>&1 | FileCheck --allow-empty --check-prefix=CHECK-WARNING %s
+// CHECK-WARNING-NOT: warning
+
+// Check that device archive 

[PATCH] D114776: [clang-offload-bundler] Reuse original file extension for device archive member

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added reviewers: saiislam, ABataev.
sdmitriev requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

This patch changes clang-offload-bundler to use the original file extension for
the device archive member when unbundling archives instead of printing a warning
and defaulting to ".o".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114776

Files:
  clang/test/Driver/fat-archive-unbundle-ext.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -180,21 +180,19 @@
   }
 };
 
-static StringRef getDeviceFileExtension(StringRef Device) {
+static StringRef getDeviceFileExtension(StringRef Device,
+StringRef BundleFileName) {
   if (Device.contains("gfx"))
 return ".bc";
   if (Device.contains("sm_"))
 return ".cubin";
-
-  WithColor::warning() << "Could not determine extension for archive"
-  "members, using \".o\"\n";
-  return ".o";
+  return sys::path::extension(BundleFileName);
 }
 
 static std::string getDeviceLibraryFileName(StringRef BundleFileName,
 StringRef Device) {
   StringRef LibName = sys::path::stem(BundleFileName);
-  StringRef Extension = getDeviceFileExtension(Device);
+  StringRef Extension = getDeviceFileExtension(Device, BundleFileName);
 
   std::string Result;
   Result += LibName;
Index: clang/test/Driver/fat-archive-unbundle-ext.c
===
--- /dev/null
+++ clang/test/Driver/fat-archive-unbundle-ext.c
@@ -0,0 +1,21 @@
+// REQUIRES: x86-registered-target
+// UNSUPPORTED: windows, darwin, aix
+
+// Generate dummy fat object
+// RUN: %clang -O0 -target %itanium_abi_triple %s -c -o %t.host.o
+// RUN: echo 'Content of device file' > %t.tgt.o
+// RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-%itanium_abi_triple 
-inputs=%t.host.o,%t.tgt.o -outputs=%t.fat.obj
+
+// Then create a static archive with that object
+// RUN: rm -f %t.fat.a
+// RUN: llvm-ar cr %t.fat.a %t.fat.obj
+
+// Unbundle device part from the archive. Check that bundler does not print 
warnings.
+// RUN: clang-offload-bundler -unbundle -type=a 
-targets=openmp-%itanium_abi_triple -inputs=%t.fat.a -outputs=%t.tgt.a 2>&1 | 
FileCheck --allow-empty --check-prefix=CHECK-WARNING %s
+// CHECK-WARNING-NOT: warning
+
+// Check that device archive member inherited file extension from the original 
file.
+// RUN: llvm-ar t %t.tgt.a | FileCheck --check-prefix=CHECK-ARCHIVE %s
+// CHECK-ARCHIVE: {{\.obj$}}
+
+void foo(void) {}


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -180,21 +180,19 @@
   }
 };
 
-static StringRef getDeviceFileExtension(StringRef Device) {
+static StringRef getDeviceFileExtension(StringRef Device,
+StringRef BundleFileName) {
   if (Device.contains("gfx"))
 return ".bc";
   if (Device.contains("sm_"))
 return ".cubin";
-
-  WithColor::warning() << "Could not determine extension for archive"
-  "members, using \".o\"\n";
-  return ".o";
+  return sys::path::extension(BundleFileName);
 }
 
 static std::string getDeviceLibraryFileName(StringRef BundleFileName,
 StringRef Device) {
   StringRef LibName = sys::path::stem(BundleFileName);
-  StringRef Extension = getDeviceFileExtension(Device);
+  StringRef Extension = getDeviceFileExtension(Device, BundleFileName);
 
   std::string Result;
   Result += LibName;
Index: clang/test/Driver/fat-archive-unbundle-ext.c
===
--- /dev/null
+++ clang/test/Driver/fat-archive-unbundle-ext.c
@@ -0,0 +1,21 @@
+// REQUIRES: x86-registered-target
+// UNSUPPORTED: windows, darwin, aix
+
+// Generate dummy fat object
+// RUN: %clang -O0 -target %itanium_abi_triple %s -c -o %t.host.o
+// RUN: echo 'Content of device file' > %t.tgt.o
+// RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-%itanium_abi_triple -inputs=%t.host.o,%t.tgt.o -outputs=%t.fat.obj
+
+// Then create a static archive with that object
+// RUN: rm -f %t.fat.a
+// RUN: llvm-ar cr %t.fat.a %t.fat.obj
+
+// Unbundle device part from the archive. Check that bundler does not print warnings.
+// RUN: clang-offload-bundler -unbundle -type=a -targets=openmp-%itanium_abi_triple -inputs=%t.fat.a 

[PATCH] D102752: [clang-offload-bundler] Delimit input/output file names by '--' for llvm-objcopy

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf8444a8e9422: [clang-offload-bundler] Delimit input/output 
file names by -- for llvm-objcopy (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102752

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -621,6 +621,7 @@
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] 
+
 "=readonly,exclude"));
 }
+ObjcopyArgs.push_back("--");
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -274,7 +274,7 @@
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck %s -DHOST=%itanium_abi_triple -DINOBJ1=%t.o 
-DINOBJ2=%t.tgt1 -DINOBJ3=%t.tgt2 -DOUTOBJ=%t.bundle3.o --check-prefix 
CK-OBJ-CMD
-// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude"
 "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude"
 "[[INOBJ1]]" "[[OUTOBJ]]"
+// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude"
 "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude"
 "--" "[[INOBJ1]]" "[[OUTOBJ]]"
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o
 // RUN: clang-offload-bundler -type=o -inputs=%t.bundle3.o -list | FileCheck 
-check-prefix=CKLST %s


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -621,6 +621,7 @@
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
 "=readonly,exclude"));
 }
+ObjcopyArgs.push_back("--");
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -274,7 +274,7 @@
 
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck %s -DHOST=%itanium_abi_triple -DINOBJ1=%t.o -DINOBJ2=%t.tgt1 -DINOBJ3=%t.tgt2 -DOUTOBJ=%t.bundle3.o --check-prefix CK-OBJ-CMD
-// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" "--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" "--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]" "--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude" "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" "--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude" "[[INOBJ1]]" "[[OUTOBJ]]"
+// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" "--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" "--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 

[PATCH] D102752: [clang-offload-bundler] Delimit input/output file names by '--' for llvm-objcopy

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added a reviewer: alexshap.
Herald added a subscriber: abrachet.
sdmitriev requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

That fixes a problem of using bundler with file names starting with dash.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102752

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -621,6 +621,7 @@
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] 
+
 "=readonly,exclude"));
 }
+ObjcopyArgs.push_back("--");
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -274,7 +274,7 @@
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck %s -DHOST=%itanium_abi_triple -DINOBJ1=%t.o 
-DINOBJ2=%t.tgt1 -DINOBJ3=%t.tgt2 -DOUTOBJ=%t.bundle3.o --check-prefix 
CK-OBJ-CMD
-// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude"
 "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude"
 "[[INOBJ1]]" "[[OUTOBJ]]"
+// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude"
 "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude"
 "--" "[[INOBJ1]]" "[[OUTOBJ]]"
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o
 // RUN: clang-offload-bundler -type=o -inputs=%t.bundle3.o -list | FileCheck 
-check-prefix=CKLST %s


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -621,6 +621,7 @@
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
 "=readonly,exclude"));
 }
+ObjcopyArgs.push_back("--");
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -274,7 +274,7 @@
 
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck %s -DHOST=%itanium_abi_triple -DINOBJ1=%t.o -DINOBJ2=%t.tgt1 -DINOBJ3=%t.tgt2 -DOUTOBJ=%t.bundle3.o --check-prefix CK-OBJ-CMD
-// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" "--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" "--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]" "--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude" "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" "--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude" "[[INOBJ1]]" "[[OUTOBJ]]"
+// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" "--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 

[PATCH] D102670: [clang-offload-bundler] Add sections and set section flags using one llvm-objcopy invocation

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8998a8aa97f8: [clang-offload-bundler] Add sections and set 
section flags using one llvm… (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102670

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -578,11 +578,7 @@
 // We will use llvm-objcopy to add target objects sections to the output
 // fat object. These sections should have 'exclude' flag set which tells
 // link editor to remove them from linker inputs when linking executable or
-// shared library. llvm-objcopy currently does not support adding new
-// section and changing flags for the added section in one invocation, and
-// because of that we have to run it two times. First run adds sections and
-// the second changes flags.
-// TODO: change it to one run once llvm-objcopy starts supporting that.
+// shared library.
 
 // Find llvm-objcopy in order to create the bundle binary.
 ErrorOr Objcopy = sys::findProgramByName(
@@ -600,14 +596,8 @@
 // Temporary files that need to be removed.
 TempFileHandlerRAII TempFiles;
 
-// Create an intermediate temporary file to save object after the first
-// llvm-objcopy run.
-Expected IntermediateObjOrErr = TempFiles.Create(None);
-if (!IntermediateObjOrErr)
-  return IntermediateObjOrErr.takeError();
-StringRef IntermediateObj = *IntermediateObjOrErr;
-
-// Compose llvm-objcopy command line for add target objects' sections.
+// Compose llvm-objcopy command line for add target objects' sections with
+// appropriate flags.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
@@ -627,20 +617,11 @@
   ObjcopyArgs.push_back(SS.save(Twine("--add-section=") +
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] 
+
 "=" + InputFile));
-}
-ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
-ObjcopyArgs.push_back(IntermediateObj);
-
-if (Error Err = executeObjcopy(*Objcopy, ObjcopyArgs))
-  return Err;
-
-// And run llvm-objcopy for the second time to update section flags.
-ObjcopyArgs.resize(1);
-for (unsigned I = 0; I < NumberOfInputs; ++I)
   ObjcopyArgs.push_back(SS.save(Twine("--set-section-flags=") +
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] 
+
 "=readonly,exclude"));
-ObjcopyArgs.push_back(IntermediateObj);
+}
+ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
 if (Error Err = executeObjcopy(*Objcopy, ObjcopyArgs))
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -274,8 +274,7 @@
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck %s -DHOST=%itanium_abi_triple -DINOBJ1=%t.o 
-DINOBJ2=%t.tgt1 -DINOBJ3=%t.tgt2 -DOUTOBJ=%t.bundle3.o --check-prefix 
CK-OBJ-CMD
-// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" 
"[[INOBJ1]]" "[[TEMPOBJ:.*]]"
-// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude"
 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude"
 "[[TEMPOBJ]]" "[[OUTOBJ]]"
+// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude"
 "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude"
 "[[INOBJ1]]" "[[OUTOBJ]]"
 
 // RUN: clang-offload-bundler -type=o 

[PATCH] D102670: [clang-offload-bundler] Add sections and set section flags using one llvm-objcopy invocation

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added reviewers: ABataev, jdoerfert.
Herald added a reviewer: alexshap.
Herald added a subscriber: abrachet.
sdmitriev requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

llvm-objcopy has been changed to support adding a section and updating section 
flags
in one run (D90438 ), so we can now change 
clang-offload-bundler to run llvm-objcopy
tool only once when creating fat object.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102670

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -578,11 +578,7 @@
 // We will use llvm-objcopy to add target objects sections to the output
 // fat object. These sections should have 'exclude' flag set which tells
 // link editor to remove them from linker inputs when linking executable or
-// shared library. llvm-objcopy currently does not support adding new
-// section and changing flags for the added section in one invocation, and
-// because of that we have to run it two times. First run adds sections and
-// the second changes flags.
-// TODO: change it to one run once llvm-objcopy starts supporting that.
+// shared library.
 
 // Find llvm-objcopy in order to create the bundle binary.
 ErrorOr Objcopy = sys::findProgramByName(
@@ -600,14 +596,8 @@
 // Temporary files that need to be removed.
 TempFileHandlerRAII TempFiles;
 
-// Create an intermediate temporary file to save object after the first
-// llvm-objcopy run.
-Expected IntermediateObjOrErr = TempFiles.Create(None);
-if (!IntermediateObjOrErr)
-  return IntermediateObjOrErr.takeError();
-StringRef IntermediateObj = *IntermediateObjOrErr;
-
-// Compose llvm-objcopy command line for add target objects' sections.
+// Compose llvm-objcopy command line for add target objects' sections with
+// appropriate flags.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
@@ -627,20 +617,11 @@
   ObjcopyArgs.push_back(SS.save(Twine("--add-section=") +
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] 
+
 "=" + InputFile));
-}
-ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
-ObjcopyArgs.push_back(IntermediateObj);
-
-if (Error Err = executeObjcopy(*Objcopy, ObjcopyArgs))
-  return Err;
-
-// And run llvm-objcopy for the second time to update section flags.
-ObjcopyArgs.resize(1);
-for (unsigned I = 0; I < NumberOfInputs; ++I)
   ObjcopyArgs.push_back(SS.save(Twine("--set-section-flags=") +
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] 
+
 "=readonly,exclude"));
-ObjcopyArgs.push_back(IntermediateObj);
+}
+ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
 if (Error Err = executeObjcopy(*Objcopy, ObjcopyArgs))
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -274,8 +274,7 @@
 
 // RUN: clang-offload-bundler -type=o 
-targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu
 -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck %s -DHOST=%itanium_abi_triple -DINOBJ1=%t.o 
-DINOBJ2=%t.tgt1 -DINOBJ3=%t.tgt2 -DOUTOBJ=%t.bundle3.o --check-prefix 
CK-OBJ-CMD
-// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" 
"[[INOBJ1]]" "[[TEMPOBJ:.*]]"
-// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude"
 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=readonly,exclude"
 "[[TEMPOBJ]]" "[[OUTOBJ]]"
+// CK-OBJ-CMD: llvm-objcopy{{(.exe)?}}" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]={{.*}}" 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__host-[[HOST]]=readonly,exclude" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 
"--set-section-flags=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=readonly,exclude"
 

[PATCH] D92010: [clang-offload-bundler] use std::forward_list for storing temp file names [NFC]

[Sergey Dmitriev 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 rG1b0ca81a6c35: [clang-offload-bundler] use std::forward_list 
for storing temp file names [NFC] (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92010

Files:
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -394,7 +395,7 @@
 if (std::error_code EC =
 sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
   return createFileError(File, EC);
-Files.push_back(File);
+Files.push_front(File);
 
 if (Contents) {
   std::error_code EC;
@@ -403,11 +404,11 @@
 return createFileError(File, EC);
   OS.write(Contents->data(), Contents->size());
 }
-return Files.back();
+return Files.front();
   }
 
 private:
-  SmallVector, 4u> Files;
+  std::forward_list> Files;
 };
 
 } // end anonymous namespace


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -394,7 +395,7 @@
 if (std::error_code EC =
 sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
   return createFileError(File, EC);
-Files.push_back(File);
+Files.push_front(File);
 
 if (Contents) {
   std::error_code EC;
@@ -403,11 +404,11 @@
 return createFileError(File, EC);
   OS.write(Contents->data(), Contents->size());
 }
-return Files.back();
+return Files.front();
   }
 
 private:
-  SmallVector, 4u> Files;
+  std::forward_list> Files;
 };
 
 } // end anonymous namespace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92010: [clang-offload-bundler] use std::forward_list for storing temp file names [NFC]

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:411
 private:
-  SmallVector, 4u> Files;
+  std::forward_list> Files;
 };

ABataev wrote:
> sdmitriev wrote:
> > ABataev wrote:
> > > What about `llvm::iplist`?
> > It can be changed to iplist as well, but why do you think it is a better 
> > option here?
> Just better to use LLVM API if possible.
Well, I do not think it will bring any benefits here compared to the 
forward_list. iplist is an intrusive list, so it requires an element to provide 
access to prev/next elements in the list, and because of that element type 
should be derived from ilist_node. So, switching to iplist would require adding 
one more class for the list element type. I agree that it is not difficult to 
do, but I just do not see why it needs to be done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92010

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

[PATCH] D92010: [clang-offload-bundler] use std::forward_list for storing temp file names [NFC]

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:411
 private:
-  SmallVector, 4u> Files;
+  std::forward_list> Files;
 };

ABataev wrote:
> What about `llvm::iplist`?
It can be changed to iplist as well, but why do you think it is a better option 
here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92010

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

[PATCH] D92010: [clang-offload-bundler] use std::forward_list for storing temp file names [NFC]

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
sdmitriev requested review of this revision.

Use a different container that preserves existing elements on modification
for storing temporary file names. Current container can make StringRefs
returned earlier invalid on reallocation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92010

Files:
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -394,7 +395,7 @@
 if (std::error_code EC =
 sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
   return createFileError(File, EC);
-Files.push_back(File);
+Files.push_front(File);
 
 if (Contents) {
   std::error_code EC;
@@ -403,11 +404,11 @@
 return createFileError(File, EC);
   OS.write(Contents->data(), Contents->size());
 }
-return Files.back();
+return Files.front();
   }
 
 private:
-  SmallVector, 4u> Files;
+  std::forward_list> Files;
 };
 
 } // end anonymous namespace


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -394,7 +395,7 @@
 if (std::error_code EC =
 sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
   return createFileError(File, EC);
-Files.push_back(File);
+Files.push_front(File);
 
 if (Contents) {
   std::error_code EC;
@@ -403,11 +404,11 @@
 return createFileError(File, EC);
   OS.write(Contents->data(), Contents->size());
 }
-return Files.back();
+return Files.front();
   }
 
 private:
-  SmallVector, 4u> Files;
+  std::forward_list> Files;
 };
 
 } // end anonymous namespace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:4554
+
+  // Language rules define if it is legal to cast from one address space
+  // to another, and which address space we should use as a "common

bader wrote:
> Anastasia wrote:
> > What language rules?
> I suppose it's a generic note and author didn't meant some particular 
> language here. This change was contributed by @sdmitriev. 
> @sdmitriev, could you clarify, please?
This comment was originally added by @asavonic to 
clang/lib/CodeGen/CGExprScalar.cpp (lines 2962-2965 in this patch), and later I 
reused his code along with the comment in this file while fixing a very similar 
issue. So, I guess it would be more correct to ask Andrew to clarify. 
@asavonic, can you please comment on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909

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

[PATCH] D73738: [Clang][Bundler][NFC] Replace SmallString<...> with StringRef

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5be2ca29217a: [Clang][Bundler][NFC] Replace 
SmallString... with StringRef (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73738

Files:
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -530,10 +530,10 @@
 
 // Create an intermediate temporary file to save object after the first
 // llvm-objcopy run.
-Expected> IntermediateObjOrErr = TempFiles.Create(None);
+Expected IntermediateObjOrErr = TempFiles.Create(None);
 if (!IntermediateObjOrErr)
   return IntermediateObjOrErr.takeError();
-const SmallString<128u>  = *IntermediateObjOrErr;
+StringRef IntermediateObj = *IntermediateObjOrErr;
 
 // Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -530,10 +530,10 @@
 
 // Create an intermediate temporary file to save object after the first
 // llvm-objcopy run.
-Expected> IntermediateObjOrErr = TempFiles.Create(None);
+Expected IntermediateObjOrErr = TempFiles.Create(None);
 if (!IntermediateObjOrErr)
   return IntermediateObjOrErr.takeError();
-const SmallString<128u>  = *IntermediateObjOrErr;
+StringRef IntermediateObj = *IntermediateObjOrErr;
 
 // Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73738: [Clang][Bundler][NFC] Replace SmallString<...> with StringRef

[Sergey Dmitriev via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73738

Files:
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -530,10 +530,10 @@
 
 // Create an intermediate temporary file to save object after the first
 // llvm-objcopy run.
-Expected> IntermediateObjOrErr = TempFiles.Create(None);
+Expected IntermediateObjOrErr = TempFiles.Create(None);
 if (!IntermediateObjOrErr)
   return IntermediateObjOrErr.takeError();
-const SmallString<128u>  = *IntermediateObjOrErr;
+StringRef IntermediateObj = *IntermediateObjOrErr;
 
 // Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -530,10 +530,10 @@
 
 // Create an intermediate temporary file to save object after the first
 // llvm-objcopy run.
-Expected> IntermediateObjOrErr = TempFiles.Create(None);
+Expected IntermediateObjOrErr = TempFiles.Create(None);
 if (!IntermediateObjOrErr)
   return IntermediateObjOrErr.takeError();
-const SmallString<128u>  = *IntermediateObjOrErr;
+StringRef IntermediateObj = *IntermediateObjOrErr;
 
 // Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73721: [Clang][Driver] Disable llvm passes for the first host OpenMP offload compilation

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG36bfdb7096cf: [Clang][Driver] Disable llvm passes for the 
first host OpenMP offload… (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73721

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/openmp-offload.c


Index: clang/test/Driver/openmp-offload.c
===
--- clang/test/Driver/openmp-offload.c
+++ clang/test/Driver/openmp-offload.c
@@ -259,7 +259,7 @@
 //
 // Generate host BC file and host object.
 //
-// CHK-COMMANDS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"
+// CHK-COMMANDS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-disable-llvm-passes"
 // CHK-COMMANDS-SAME: 
"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu"
 // CHK-COMMANDS-SAME: "-o" "
 // CHK-COMMANDS-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "c" "
@@ -269,7 +269,7 @@
 // CHK-COMMANDS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-E" {{.*}}"-fopenmp" {{.*}}"-o" "
 // CHK-COMMANDS-ST-SAME: [[HOSTPP:[^\\/]+\.i]]" "-x" "c" "
 // CHK-COMMANDS-ST-SAME: [[INPUT:[^\\/]+\.c]]"
-// CHK-COMMANDS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-COMMANDS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-COMMANDS-ST-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "cpp-output" 
"{{.*}}[[HOSTPP]]"
 // CHK-COMMANDS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-S" {{.*}}"-fopenmp" {{.*}}"-o" "
 // CHK-COMMANDS-ST-SAME: [[HOSTASM:[^\\/]+\.s]]" "-x" "ir" "{{.*}}[[HOSTBC]]"
@@ -427,14 +427,14 @@
 // RUN:   | FileCheck -check-prefix=CHK-BUJOBS-ST %s
 
 // Create host BC.
-// CHK-BUJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"  {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-BUJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-BUJOBS-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "c" "
 // CHK-BUJOBS-SAME: [[INPUT:[^\\/]+\.c]]"
 
 // CHK-BUJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-E"  {{.*}}"-fopenmp" {{.*}}"-o" "
 // CHK-BUJOBS-ST-SAME: [[HOSTPP:[^\\/]+\.i]]" "-x" "c" "
 // CHK-BUJOBS-ST-SAME: [[INPUT:[^\\/]+\.c]]"
-// CHK-BUJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"  {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-BUJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-BUJOBS-ST-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "cpp-output" 
"{{.*}}[[HOSTPP]]"
 
 // Create target 1 object.
@@ -493,7 +493,7 @@
 // CHK-UBJOBS-SAME: [[HOSTPP:[^\\/]+\.i]],
 // CHK-UBJOBS-SAME: [[T1PP:[^\\/]+\.i]],
 // CHK-UBJOBS-SAME: [[T2PP:[^\\/]+\.i]]" "-unbundle"
-// CHK-UBJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"  {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-UBJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-UBJOBS-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "cpp-output" 
"{{.*}}[[HOSTPP]]"
 // CHK-UBJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-obj" {{.*}}"-fopenmp" {{.*}}"-o" "
 // CHK-UBJOBS-SAME: [[HOSTOBJ:[^\\/]+\.o]]" "-x" "ir" "{{.*}}[[HOSTBC]]"
@@ -502,7 +502,7 @@
 // CHK-UBJOBS-ST-SAME: [[HOSTPP:[^\\/,]+\.i]],
 // CHK-UBJOBS-ST-SAME: [[T1PP:[^\\/,]+\.i]],
 // CHK-UBJOBS-ST-SAME: [[T2PP:[^\\/,]+\.i]]" "-unbundle"
-// CHK-UBJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"  {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-UBJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-UBJOBS-ST-SAME: [[HOSTBC:[^\\/]+\.bc]]" 

[PATCH] D73721: [Clang][Driver] Disable llvm passes for the first host OpenMP offload compilation

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added reviewers: ABataev, jdoerfert, hfinkel.
Herald added subscribers: cfe-commits, guansong.
Herald added a project: clang.

With OpenMP offloading host compilation is done in two phases to capture host 
IR that is passed to all device compilations as input. But it turns out that we 
currently run entire LLVM optimization pipeline on host IR on both compilations 
which may have unpredictable effects on the resulting code. This patch fixes 
this problem by disabling LLVM passes on the first compilation, so the host IR 
that is passed to device compilations will be captured right after front end.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73721

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/openmp-offload.c


Index: clang/test/Driver/openmp-offload.c
===
--- clang/test/Driver/openmp-offload.c
+++ clang/test/Driver/openmp-offload.c
@@ -259,7 +259,7 @@
 //
 // Generate host BC file and host object.
 //
-// CHK-COMMANDS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"
+// CHK-COMMANDS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-disable-llvm-passes"
 // CHK-COMMANDS-SAME: 
"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu"
 // CHK-COMMANDS-SAME: "-o" "
 // CHK-COMMANDS-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "c" "
@@ -269,7 +269,7 @@
 // CHK-COMMANDS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-E" {{.*}}"-fopenmp" {{.*}}"-o" "
 // CHK-COMMANDS-ST-SAME: [[HOSTPP:[^\\/]+\.i]]" "-x" "c" "
 // CHK-COMMANDS-ST-SAME: [[INPUT:[^\\/]+\.c]]"
-// CHK-COMMANDS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-COMMANDS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-COMMANDS-ST-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "cpp-output" 
"{{.*}}[[HOSTPP]]"
 // CHK-COMMANDS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-S" {{.*}}"-fopenmp" {{.*}}"-o" "
 // CHK-COMMANDS-ST-SAME: [[HOSTASM:[^\\/]+\.s]]" "-x" "ir" "{{.*}}[[HOSTBC]]"
@@ -427,14 +427,14 @@
 // RUN:   | FileCheck -check-prefix=CHK-BUJOBS-ST %s
 
 // Create host BC.
-// CHK-BUJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"  {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-BUJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-BUJOBS-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "c" "
 // CHK-BUJOBS-SAME: [[INPUT:[^\\/]+\.c]]"
 
 // CHK-BUJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-E"  {{.*}}"-fopenmp" {{.*}}"-o" "
 // CHK-BUJOBS-ST-SAME: [[HOSTPP:[^\\/]+\.i]]" "-x" "c" "
 // CHK-BUJOBS-ST-SAME: [[INPUT:[^\\/]+\.c]]"
-// CHK-BUJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"  {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-BUJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-BUJOBS-ST-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "cpp-output" 
"{{.*}}[[HOSTPP]]"
 
 // Create target 1 object.
@@ -493,7 +493,7 @@
 // CHK-UBJOBS-SAME: [[HOSTPP:[^\\/]+\.i]],
 // CHK-UBJOBS-SAME: [[T1PP:[^\\/]+\.i]],
 // CHK-UBJOBS-SAME: [[T2PP:[^\\/]+\.i]]" "-unbundle"
-// CHK-UBJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"  {{.*}}"-fopenmp" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
+// CHK-UBJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc" {{.*}}"-fopenmp" {{.*}}"-disable-llvm-passes" 
{{.*}}"-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu" 
{{.*}}"-o" "
 // CHK-UBJOBS-SAME: [[HOSTBC:[^\\/]+\.bc]]" "-x" "cpp-output" 
"{{.*}}[[HOSTPP]]"
 // CHK-UBJOBS: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-obj" {{.*}}"-fopenmp" {{.*}}"-o" "
 // CHK-UBJOBS-SAME: [[HOSTOBJ:[^\\/]+\.o]]" "-x" "ir" "{{.*}}[[HOSTBC]]"
@@ -502,7 +502,7 @@
 // CHK-UBJOBS-ST-SAME: [[HOSTPP:[^\\/,]+\.i]],
 // CHK-UBJOBS-ST-SAME: [[T1PP:[^\\/,]+\.i]],
 // CHK-UBJOBS-ST-SAME: [[T2PP:[^\\/,]+\.i]]" "-unbundle"
-// CHK-UBJOBS-ST: clang{{.*}}" "-cc1" "-triple" "powerpc64le-unknown-linux" 
{{.*}}"-emit-llvm-bc"  

[PATCH] D73642: [Clang][Bundler] Reduce fat object size

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc53cb2bdc78e: [Clang][Bundler] Reduce fat object size 
(authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73642

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -30,7 +30,6 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
-#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
@@ -365,6 +364,41 @@
   }
 };
 
+namespace {
+
+// This class implements a list of temporary files that are removed upon
+// object destruction.
+class TempFileHandlerRAII {
+public:
+  ~TempFileHandlerRAII() {
+for (const auto  : Files)
+  sys::fs::remove(File);
+  }
+
+  // Creates temporary file with given contents.
+  Expected Create(Optional> Contents) {
+SmallString<128u> File;
+if (std::error_code EC =
+sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
+  return createFileError(File, EC);
+Files.push_back(File);
+
+if (Contents) {
+  std::error_code EC;
+  raw_fd_ostream OS(File, EC);
+  if (EC)
+return createFileError(File, EC);
+  OS.write(Contents->data(), Contents->size());
+}
+return Files.back();
+  }
+
+private:
+  SmallVector, 4u> Files;
+};
+
+} // end anonymous namespace
+
 /// Handler for object files. The bundles are organized by sections with a
 /// designated name.
 ///
@@ -433,11 +467,16 @@
   Error ReadBundleEnd(MemoryBuffer ) final { return Error::success(); }
 
   Error ReadBundle(raw_fd_ostream , MemoryBuffer ) final {
-Expected Content = CurrentSection->getContents();
-if (!Content)
-  return Content.takeError();
+Expected ContentOrErr = CurrentSection->getContents();
+if (!ContentOrErr)
+  return ContentOrErr.takeError();
+StringRef Content = *ContentOrErr;
+
+// Copy fat object contents to the output when extracting host bundle.
+if (Content.size() == 1u && Content.front() == 0)
+  Content = StringRef(Input.getBufferStart(), Input.getBufferSize());
 
-OS.write(Content->data(), Content->size());
+OS.write(Content.data(), Content.size());
 return Error::success();
   }
 
@@ -486,22 +525,37 @@
 // to pass down to llvm-objcopy.
 OS.close();
 
+// Temporary files that need to be removed.
+TempFileHandlerRAII TempFiles;
+
 // Create an intermediate temporary file to save object after the first
 // llvm-objcopy run.
-SmallString<128u> IntermediateObj;
-if (std::error_code EC = sys::fs::createTemporaryFile(
-"clang-offload-bundler", "tmp", IntermediateObj))
-  return createFileError(IntermediateObj, EC);
-FileRemover IntermediateObjRemover(IntermediateObj);
+Expected> IntermediateObjOrErr = TempFiles.Create(None);
+if (!IntermediateObjOrErr)
+  return IntermediateObjOrErr.takeError();
+const SmallString<128u>  = *IntermediateObjOrErr;
 
 // Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
-for (unsigned I = 0; I < NumberOfInputs; ++I)
+for (unsigned I = 0; I < NumberOfInputs; ++I) {
+  StringRef InputFile = InputFileNames[I];
+  if (I == HostInputIndex) {
+// Special handling for the host bundle. We do not need to add a
+// standard bundle for the host object since we are going to use fat
+// object as a host object. Therefore use dummy contents (one zero byte)
+// when creating section for the host bundle.
+Expected TempFileOrErr = TempFiles.Create(ArrayRef(0));
+if (!TempFileOrErr)
+  return TempFileOrErr.takeError();
+InputFile = *TempFileOrErr;
+  }
+
   ObjcopyArgs.push_back(SS.save(Twine("--add-section=") +
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
-"=" + InputFileNames[I]));
+"=" + InputFile));
+}
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(IntermediateObj);
 
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -253,16 +253,16 @@
 
 // RUN: clang-offload-bundler -type=o 

[PATCH] D73642: [Clang][Bundler] Reduce fat object size

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

@ABataev, do you have any additional comments?


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

https://reviews.llvm.org/D73642



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

[PATCH] D73642: [Clang][Bundler] Reduce fat object size

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:378
+  // Creates temporary file with given contents.
+  Expected Create(Optional> Contents) {
+SmallString<128u> File;

ABataev wrote:
> I see that Contents is either `None` or just one byte `0`. Maybe, better to 
> use bool flag here instead of `Contents`?
Well, maybe we will need some other contents besides one zero byte in future, 
so I think having optional contents is probably better.


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

https://reviews.llvm.org/D73642



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

[PATCH] D73642: [Clang][Bundler] Reduce fat object size

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 241278.
sdmitriev added a comment.

Addressed review comments.


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

https://reviews.llvm.org/D73642

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -30,7 +30,6 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
-#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
@@ -365,6 +364,41 @@
   }
 };
 
+namespace {
+
+// This class implements a list of temporary files that are removed upon
+// object destruction.
+class TempFileHandlerRAII {
+public:
+  ~TempFileHandlerRAII() {
+for (const auto  : Files)
+  sys::fs::remove(File);
+  }
+
+  // Creates temporary file with given contents.
+  Expected Create(Optional> Contents) {
+SmallString<128u> File;
+if (std::error_code EC =
+sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
+  return createFileError(File, EC);
+Files.push_back(File);
+
+if (Contents) {
+  std::error_code EC;
+  raw_fd_ostream OS(File, EC);
+  if (EC)
+return createFileError(File, EC);
+  OS.write(Contents->data(), Contents->size());
+}
+return Files.back();
+  }
+
+private:
+  SmallVector, 4u> Files;
+};
+
+} // end anonymous namespace
+
 /// Handler for object files. The bundles are organized by sections with a
 /// designated name.
 ///
@@ -433,11 +467,16 @@
   Error ReadBundleEnd(MemoryBuffer ) final { return Error::success(); }
 
   Error ReadBundle(raw_fd_ostream , MemoryBuffer ) final {
-Expected Content = CurrentSection->getContents();
-if (!Content)
-  return Content.takeError();
+Expected ContentOrErr = CurrentSection->getContents();
+if (!ContentOrErr)
+  return ContentOrErr.takeError();
+StringRef Content = *ContentOrErr;
+
+// Copy fat object contents to the output when extracting host bundle.
+if (Content.size() == 1u && Content.front() == 0)
+  Content = StringRef(Input.getBufferStart(), Input.getBufferSize());
 
-OS.write(Content->data(), Content->size());
+OS.write(Content.data(), Content.size());
 return Error::success();
   }
 
@@ -486,22 +525,37 @@
 // to pass down to llvm-objcopy.
 OS.close();
 
+// Temporary files that need to be removed.
+TempFileHandlerRAII TempFiles;
+
 // Create an intermediate temporary file to save object after the first
 // llvm-objcopy run.
-SmallString<128u> IntermediateObj;
-if (std::error_code EC = sys::fs::createTemporaryFile(
-"clang-offload-bundler", "tmp", IntermediateObj))
-  return createFileError(IntermediateObj, EC);
-FileRemover IntermediateObjRemover(IntermediateObj);
+Expected> IntermediateObjOrErr = TempFiles.Create(None);
+if (!IntermediateObjOrErr)
+  return IntermediateObjOrErr.takeError();
+const SmallString<128u>  = *IntermediateObjOrErr;
 
 // Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
-for (unsigned I = 0; I < NumberOfInputs; ++I)
+for (unsigned I = 0; I < NumberOfInputs; ++I) {
+  StringRef InputFile = InputFileNames[I];
+  if (I == HostInputIndex) {
+// Special handling for the host bundle. We do not need to add a
+// standard bundle for the host object since we are going to use fat
+// object as a host object. Therefore use dummy contents (one zero byte)
+// when creating section for the host bundle.
+Expected TempFileOrErr = TempFiles.Create(ArrayRef(0));
+if (!TempFileOrErr)
+  return TempFileOrErr.takeError();
+InputFile = *TempFileOrErr;
+  }
+
   ObjcopyArgs.push_back(SS.save(Twine("--add-section=") +
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
-"=" + InputFileNames[I]));
+"=" + InputFile));
+}
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(IntermediateObj);
 
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -253,16 +253,16 @@
 
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 

[PATCH] D73642: [Clang][Bundler] Reduce fat object size

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 241269.
sdmitriev added a comment.

Addressed review comments.


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

https://reviews.llvm.org/D73642

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -30,7 +30,6 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
-#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
@@ -365,6 +364,40 @@
   }
 };
 
+namespace {
+
+// This structure implements a list of temporary files that are removed upon
+// object destruction.
+struct TempFileList {
+  ~TempFileList() {
+for (const auto  : Files)
+  sys::fs::remove(File);
+  }
+
+  // Creates temporary file with given contents.
+  Expected Create(Optional> Contents) {
+SmallString<128u> File;
+if (std::error_code EC =
+sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
+  return createFileError(File, EC);
+Files.push_back(File);
+
+if (Contents) {
+  std::error_code EC;
+  raw_fd_ostream OS(File, EC);
+  if (EC)
+return createFileError(File, EC);
+  OS.write(Contents->data(), Contents->size());
+}
+return Files.back();
+  }
+
+private:
+  SmallVector, 4u> Files;
+};
+
+} // end anonymous namespace
+
 /// Handler for object files. The bundles are organized by sections with a
 /// designated name.
 ///
@@ -433,11 +466,16 @@
   Error ReadBundleEnd(MemoryBuffer ) final { return Error::success(); }
 
   Error ReadBundle(raw_fd_ostream , MemoryBuffer ) final {
-Expected Content = CurrentSection->getContents();
-if (!Content)
-  return Content.takeError();
+Expected ContentOrErr = CurrentSection->getContents();
+if (!ContentOrErr)
+  return ContentOrErr.takeError();
+StringRef Content = *ContentOrErr;
+
+// Copy fat object contents to the output when extracting host bundle.
+if (Content.size() == 1u && Content.front() == 0)
+  Content = StringRef(Input.getBufferStart(), Input.getBufferSize());
 
-OS.write(Content->data(), Content->size());
+OS.write(Content.data(), Content.size());
 return Error::success();
   }
 
@@ -486,22 +524,37 @@
 // to pass down to llvm-objcopy.
 OS.close();
 
+// Temporary files that need to be removed.
+TempFileList TempFiles;
+
 // Create an intermediate temporary file to save object after the first
 // llvm-objcopy run.
-SmallString<128u> IntermediateObj;
-if (std::error_code EC = sys::fs::createTemporaryFile(
-"clang-offload-bundler", "tmp", IntermediateObj))
-  return createFileError(IntermediateObj, EC);
-FileRemover IntermediateObjRemover(IntermediateObj);
+Expected> IntermediateObjOrErr = TempFiles.Create(None);
+if (!IntermediateObjOrErr)
+  return IntermediateObjOrErr.takeError();
+const SmallString<128u>  = *IntermediateObjOrErr;
 
 // Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
-for (unsigned I = 0; I < NumberOfInputs; ++I)
+for (unsigned I = 0; I < NumberOfInputs; ++I) {
+  StringRef InputFile = InputFileNames[I];
+  if (I == HostInputIndex) {
+// Special handling for the host bundle. We do not need to add a
+// standard bundle for the host object since we are going to use fat
+// object as a host object. Therefore use dummy contents (one zero byte)
+// when creating section for the host bundle.
+Expected TempFileOrErr = TempFiles.Create(ArrayRef(0));
+if (!TempFileOrErr)
+  return TempFileOrErr.takeError();
+InputFile = *TempFileOrErr;
+  }
+
   ObjcopyArgs.push_back(SS.save(Twine("--add-section=") +
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
-"=" + InputFileNames[I]));
+"=" + InputFile));
+}
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
 ObjcopyArgs.push_back(IntermediateObj);
 
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -253,16 +253,16 @@
 
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck 

[PATCH] D73642: [Clang][Bundler] Reduce fat object size

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added a reviewer: alexshap.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fat object size has significantly increased after D65819 
 which changed bundler tool to add host object 
as a normal bundle to the fat output which almost doubled its size. That patch 
was fixing the following issues

1. Problems associated with the partial linking - global constructors were not 
called for partially linking objects which clearly resulted in incorrect 
behavior.
2. Eliminating "junk" target object sections from the linked binary on the host 
side.

The first problem is no longer relevant because we do not use partial linking 
for creating fat objects anymore. Target objects sections are now inserted into 
the resulting fat object with a help of llvm-objcopy tool.

The second issue, "junk" sections in the linked host binary, has been fixed in 
D73408  by adding "exclude" flag to the fat 
object's sections which contain target objects. This flag tells linker to drop 
section from the inputs when linking executable or shared library, therefore 
these sections will not be propagated in the linked binary.

Since both problems have been solved, we can revert D65819 
 changes to reduce fat object size and this 
patch essentially is doing that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73642

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -30,7 +30,6 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
-#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
@@ -433,11 +432,16 @@
   Error ReadBundleEnd(MemoryBuffer ) final { return Error::success(); }
 
   Error ReadBundle(raw_fd_ostream , MemoryBuffer ) final {
-Expected Content = CurrentSection->getContents();
-if (!Content)
-  return Content.takeError();
+Expected ContentOrErr = CurrentSection->getContents();
+if (!ContentOrErr)
+  return ContentOrErr.takeError();
+StringRef Content = *ContentOrErr;
 
-OS.write(Content->data(), Content->size());
+// Copy fat object contents to the output when extracting host bundle.
+if (Content.size() == 1u && Content.front() == 0)
+  Content = StringRef(Input.getBufferStart(), Input.getBufferSize());
+
+OS.write(Content.data(), Content.size());
 return Error::success();
   }
 
@@ -486,22 +490,60 @@
 // to pass down to llvm-objcopy.
 OS.close();
 
+// Temporary files that need to be removed.
+struct TempFileList : public SmallVector, 2u> {
+  ~TempFileList() {
+for (const auto  : *this)
+  sys::fs::remove(File);
+clear();
+  }
+
+  Expected Create(Optional> Contents) {
+SmallString<128u> FileName;
+if (std::error_code EC = sys::fs::createTemporaryFile(
+"clang-offload-bundler", "tmp", FileName))
+  return createFileError(FileName, EC);
+push_back(FileName);
+
+if (Contents) {
+  std::error_code EC;
+  raw_fd_ostream HostFile(FileName, EC);
+  if (EC)
+return createFileError(FileName, EC);
+  HostFile.write(Contents->data(), Contents->size());
+}
+return back();
+  }
+} TempFiles;
+
 // Create an intermediate temporary file to save object after the first
 // llvm-objcopy run.
-SmallString<128u> IntermediateObj;
-if (std::error_code EC = sys::fs::createTemporaryFile(
-"clang-offload-bundler", "tmp", IntermediateObj))
-  return createFileError(IntermediateObj, EC);
-FileRemover IntermediateObjRemover(IntermediateObj);
+Expected> IntermediateObjOrErr = TempFiles.Create(None);
+if (!IntermediateObjOrErr)
+  return IntermediateObjOrErr.takeError();
+const SmallString<128u>  = *IntermediateObjOrErr;
 
 // Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
-for (unsigned I = 0; I < NumberOfInputs; ++I)
+for (unsigned I = 0; I < NumberOfInputs; ++I) {
+  StringRef InputFile = InputFileNames[I];
+  if (I == HostInputIndex) {
+// Special handling for the host bundle. We do not need to add a
+// standard bundle for the host object since we are going to use fat
+// object as a host object. 

[PATCH] D73408: [Clang][Bundler] Add 'exclude' flag to target objects sections

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6e82d0dfd8df: [Clang][Bundler] Add exclude flag 
to target objects sections (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73408

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -30,6 +30,7 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
@@ -463,6 +464,15 @@
 if (NumberOfProcessedInputs != NumberOfInputs)
   return Error::success();
 
+// We will use llvm-objcopy to add target objects sections to the output
+// fat object. These sections should have 'exclude' flag set which tells
+// link editor to remove them from linker inputs when linking executable or
+// shared library. llvm-objcopy currently does not support adding new
+// section and changing flags for the added section in one invocation, and
+// because of that we have to run it two times. First run adds sections and
+// the second changes flags.
+// TODO: change it to one run once llvm-objcopy starts supporting that.
+
 // Find llvm-objcopy in order to create the bundle binary.
 ErrorOr Objcopy = sys::findProgramByName(
 "llvm-objcopy", sys::path::parent_path(BundlerExecutable));
@@ -476,7 +486,15 @@
 // to pass down to llvm-objcopy.
 OS.close();
 
-// Compose command line for the objcopy tool.
+// Create an intermediate temporary file to save object after the first
+// llvm-objcopy run.
+SmallString<128u> IntermediateObj;
+if (std::error_code EC = sys::fs::createTemporaryFile(
+"clang-offload-bundler", "tmp", IntermediateObj))
+  return createFileError(IntermediateObj, EC);
+FileRemover IntermediateObjRemover(IntermediateObj);
+
+// Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
@@ -485,25 +503,44 @@
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
 "=" + InputFileNames[I]));
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
+ObjcopyArgs.push_back(IntermediateObj);
+
+if (Error Err = executeObjcopy(*Objcopy, ObjcopyArgs))
+  return Err;
+
+// And run llvm-objcopy for the second time to update section flags.
+ObjcopyArgs.resize(1);
+for (unsigned I = 0; I < NumberOfInputs; ++I)
+  ObjcopyArgs.push_back(SS.save(Twine("--set-section-flags=") +
+OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
+"=readonly,exclude"));
+ObjcopyArgs.push_back(IntermediateObj);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
-// If the user asked for the commands to be printed out, we do that instead
-// of executing it.
+if (Error Err = executeObjcopy(*Objcopy, ObjcopyArgs))
+  return Err;
+
+return Error::success();
+  }
+
+  Error WriteBundle(raw_fd_ostream , MemoryBuffer ) final {
+return Error::success();
+  }
+
+private:
+  static Error executeObjcopy(StringRef Objcopy, ArrayRef Args) {
+// If the user asked for the commands to be printed out, we do that
+// instead of executing it.
 if (PrintExternalCommands) {
-  errs() << "\"" << *Objcopy << "\"";
-  for (StringRef Arg : drop_begin(ObjcopyArgs, 1))
+  errs() << "\"" << Objcopy << "\"";
+  for (StringRef Arg : drop_begin(Args, 1))
 errs() << " \"" << Arg << "\"";
   errs() << "\n";
 } else {
-  if (sys::ExecuteAndWait(*Objcopy, ObjcopyArgs))
+  if (sys::ExecuteAndWait(Objcopy, Args))
 return createStringError(inconvertibleErrorCode(),
  "'llvm-objcopy' tool failed");
 }
-
-return Error::success();
-  }
-
-  Error WriteBundle(raw_fd_ostream , MemoryBuffer ) final {
 return Error::success();
   }
 };
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -253,7 +253,8 @@
 
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck %s 

[PATCH] D73408: [Clang][Bundler] Add 'exclude' flag to target objects sections

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Any additional comments?


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

https://reviews.llvm.org/D73408



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

[PATCH] D73408: [Clang][Bundler] Add 'exclude' flag to target objects sections

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 240676.
sdmitriev added a comment.

Addressed review comments.


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

https://reviews.llvm.org/D73408

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -30,6 +30,7 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
@@ -463,6 +464,15 @@
 if (NumberOfProcessedInputs != NumberOfInputs)
   return Error::success();
 
+// We will use llvm-objcopy to add target objects sections to the output
+// fat object. These sections should have 'exclude' flag set which tells
+// link editor to remove them from linker inputs when linking executable or
+// shared library. llvm-objcopy currently does not support adding new
+// section and changing flags for the added section in one invocation, and
+// because of that we have to run it two times. First run adds sections and
+// the second changes flags.
+// TODO: change it to one run once llvm-objcopy starts supporting that.
+
 // Find llvm-objcopy in order to create the bundle binary.
 ErrorOr Objcopy = sys::findProgramByName(
 "llvm-objcopy", sys::path::parent_path(BundlerExecutable));
@@ -476,7 +486,15 @@
 // to pass down to llvm-objcopy.
 OS.close();
 
-// Compose command line for the objcopy tool.
+// Create an intermediate temporary file to save object after the first
+// llvm-objcopy run.
+SmallString<128u> IntermediateObj;
+if (std::error_code EC = sys::fs::createTemporaryFile(
+"clang-offload-bundler", "tmp", IntermediateObj))
+  return createFileError(IntermediateObj, EC);
+FileRemover IntermediateObjRemover(IntermediateObj);
+
+// Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
@@ -485,25 +503,44 @@
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
 "=" + InputFileNames[I]));
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
+ObjcopyArgs.push_back(IntermediateObj);
+
+if (Error Err = executeObjcopy(*Objcopy, ObjcopyArgs))
+  return Err;
+
+// And run llvm-objcopy for the second time to update section flags.
+ObjcopyArgs.resize(1);
+for (unsigned I = 0; I < NumberOfInputs; ++I)
+  ObjcopyArgs.push_back(SS.save(Twine("--set-section-flags=") +
+OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
+"=readonly,exclude"));
+ObjcopyArgs.push_back(IntermediateObj);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
-// If the user asked for the commands to be printed out, we do that instead
-// of executing it.
+if (Error Err = executeObjcopy(*Objcopy, ObjcopyArgs))
+  return Err;
+
+return Error::success();
+  }
+
+  Error WriteBundle(raw_fd_ostream , MemoryBuffer ) final {
+return Error::success();
+  }
+
+private:
+  static Error executeObjcopy(StringRef Objcopy, ArrayRef Args) {
+// If the user asked for the commands to be printed out, we do that
+// instead of executing it.
 if (PrintExternalCommands) {
-  errs() << "\"" << *Objcopy << "\"";
-  for (StringRef Arg : drop_begin(ObjcopyArgs, 1))
+  errs() << "\"" << Objcopy << "\"";
+  for (StringRef Arg : drop_begin(Args, 1))
 errs() << " \"" << Arg << "\"";
   errs() << "\n";
 } else {
-  if (sys::ExecuteAndWait(*Objcopy, ObjcopyArgs))
+  if (sys::ExecuteAndWait(Objcopy, Args))
 return createStringError(inconvertibleErrorCode(),
  "'llvm-objcopy' tool failed");
 }
-
-return Error::success();
-  }
-
-  Error WriteBundle(raw_fd_ostream , MemoryBuffer ) final {
 return Error::success();
   }
 };
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -253,7 +253,8 @@
 
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -inputs=%t.o,%t.tgt1,%t.tgt2 -outputs=%t.bundle3.o -### 2>&1 \
 // RUN: | FileCheck %s -DHOST=%itanium_abi_triple -DINOBJ1=%t.o -DINOBJ2=%t.tgt1 -DINOBJ3=%t.tgt2 -DOUTOBJ=%t.bundle3.o --check-prefix CK-OBJ-CMD
-// CK-OBJ-CMD: 

[PATCH] D73408: [Clang][Bundler] Add 'exclude' flag to target objects sections

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:467-475
+// We will use llvm-objcopy to add target objects’ sections to the output
+// fat object. These sections should have ‘exclude’ flag set which tells
+// link editor to remove them from linker inputs when linking executable or
+// shared library. llvm-objcopy currently does not support adding new
+// section and changing flags for the added section in one invocation, and
+// because of that we have to run it two times. First run adds sections and
+// the second changes flags. TODO: change it to one run once llvm-objcopy

ABataev wrote:
> 1. Extra spaces in the comment.
> 2. TODO better to have on a separate line.
> 3. Maybe it is better to postpone it until llvm-objcopy can do this in one 
> run?
1. Will fix.
2. Ok.
3. I do not think that anybody is working on that problem now, so it is not 
clear how much time that will take. Also merging two llvm-objcopy invocations 
into one will be a simple NFC change, so I think this should not block this 
patch.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:527
+
+// And run llvm-objcopy fro the second time to update section flags.
+ObjcopyArgs.resize(1);

ABataev wrote:
> from
This was supposed to be 'for'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73408



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

[PATCH] D73408: [Clang][Bundler] Add 'exclude' flag to target objects sections

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added a reviewer: alexshap.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This flag tells link editor to exclude section from linker inputs when linking 
executable or shared library.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73408

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -30,6 +30,7 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
@@ -463,6 +464,15 @@
 if (NumberOfProcessedInputs != NumberOfInputs)
   return Error::success();
 
+// We will use llvm-objcopy to add target objects’ sections to the output
+// fat object. These sections should have ‘exclude’ flag set which tells
+// link editor to remove them from linker inputs when linking executable or
+// shared library. llvm-objcopy currently does not support adding new
+// section and changing flags for the added section in one invocation, and
+// because of that we have to run it two times. First run adds sections and
+// the second changes flags. TODO: change it to one run once llvm-objcopy
+// starts supporting that.
+
 // Find llvm-objcopy in order to create the bundle binary.
 ErrorOr Objcopy = sys::findProgramByName(
 "llvm-objcopy", sys::path::parent_path(BundlerExecutable));
@@ -476,7 +486,15 @@
 // to pass down to llvm-objcopy.
 OS.close();
 
-// Compose command line for the objcopy tool.
+// Create an intermediate temporary file to save object after the first
+// llvm-objcopy run.
+SmallString<128u> IntermediateObj;
+if (std::error_code EC = sys::fs::createTemporaryFile(
+"clang-offload-bundler", "tmp", IntermediateObj))
+  return createFileError(IntermediateObj, EC);
+FileRemover IntermediateObjRemover(IntermediateObj);
+
+// Compose llvm-objcopy command line for add target objects' sections.
 BumpPtrAllocator Alloc;
 StringSaver SS{Alloc};
 SmallVector ObjcopyArgs{"llvm-objcopy"};
@@ -485,20 +503,38 @@
 OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
 "=" + InputFileNames[I]));
 ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
+ObjcopyArgs.push_back(IntermediateObj);
+
+auto ExecuteObjcopy = [](ArrayRef ObjcopyArgs) -> Error {
+  // If the user asked for the commands to be printed out, we do that
+  // instead of executing it.
+  if (PrintExternalCommands) {
+errs() << "\"" << *Objcopy << "\"";
+for (StringRef Arg : drop_begin(ObjcopyArgs, 1))
+  errs() << " \"" << Arg << "\"";
+errs() << "\n";
+  } else {
+if (sys::ExecuteAndWait(*Objcopy, ObjcopyArgs))
+  return createStringError(inconvertibleErrorCode(),
+   "'llvm-objcopy' tool failed");
+  }
+  return Error::success();
+};
+
+if (Error Err = ExecuteObjcopy(ObjcopyArgs))
+  return Err;
+
+// And run llvm-objcopy fro the second time to update section flags.
+ObjcopyArgs.resize(1);
+for (unsigned I = 0; I < NumberOfInputs; ++I)
+  ObjcopyArgs.push_back(SS.save(Twine("--set-section-flags=") +
+OFFLOAD_BUNDLER_MAGIC_STR + TargetNames[I] +
+"=readonly,exclude"));
+ObjcopyArgs.push_back(IntermediateObj);
 ObjcopyArgs.push_back(OutputFileNames.front());
 
-// If the user asked for the commands to be printed out, we do that instead
-// of executing it.
-if (PrintExternalCommands) {
-  errs() << "\"" << *Objcopy << "\"";
-  for (StringRef Arg : drop_begin(ObjcopyArgs, 1))
-errs() << " \"" << Arg << "\"";
-  errs() << "\n";
-} else {
-  if (sys::ExecuteAndWait(*Objcopy, ObjcopyArgs))
-return createStringError(inconvertibleErrorCode(),
- "'llvm-objcopy' tool failed");
-}
+if (Error Err = ExecuteObjcopy(ObjcopyArgs))
+  return Err;
 
 return Error::success();
   }
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -253,7 +253,8 @@
 
 // RUN: clang-offload-bundler -type=o 

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: llvm/test/Other/opt-O2-pipeline.ll:1
-; RUN: opt -mtriple=x86_64-- -O2 -debug-pass=Structure < %s -o /dev/null 2>&1 
| FileCheck %s
+; RUN: opt -mtriple=x86_64-- -O2 -debug-pass=Structure < %s -o /dev/null 2>&1 
| FileCheck --check-prefixes=CHECK-,%llvmcheckext %s 
 

Looks like there is a redundant '-' at the end of the check prefix that need to 
be removed (CHECK- => CHECK). And opt-O3-pipeline.ll and opt-Os-pipeline.ll 
tests also have this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



___
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.

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

[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

[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

[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

[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

[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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdd501045cdea: [Clang][Bundler] Error reporting improvements 
(authored by sdmitriev).

Changed prior to commit:
  https://reviews.llvm.org/D67031?vs=224726=226521#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67031

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -26,15 +26,17 @@
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include 
 #include 
 #include 
@@ -42,6 +44,7 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace llvm;
 using namespace llvm::object;
@@ -123,36 +126,37 @@
   virtual ~FileHandler() {}
 
   /// Update the file handler with information from the header of the bundled
-  /// file
-  virtual void ReadHeader(MemoryBuffer ) = 0;
+  /// file.
+  virtual Error ReadHeader(MemoryBuffer ) = 0;
 
-  /// Read the marker of the next bundled to be read in the file. The triple of
-  /// the target associated with that bundle is returned. An empty string is
-  /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  /// Read the marker of the next bundled to be read in the file. The bundle
+  /// name is returned if there is one in the file, or `None` if there are no
+  /// more bundles to be read.
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 
   /// Write the header of the bundled file to \a OS based on the information
   /// gathered from \a Inputs.
-  virtual void WriteHeader(raw_fd_ostream ,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
 
   /// Write the marker that closes a bundle for the triple \a TargetTriple to \a
-  /// OS. Return true if any error was found.
-
-  virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -224,7 +228,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer ) final {
+  Error ReadHeader(MemoryBuffer ) final {
 StringRef FC = Input.getBuffer();
 
 // Initialize the current bundle with the end of the container.
@@ -233,16 +237,16 @@
 // Check if buffer is smaller than magic string.
 size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
 if (ReadChars > FC.size())
-  return;
+  return Error::success();
 
 // Check if no magic was found.
 StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
 if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
-  return;
+  return Error::success();
 
 // Read number of bundles.
 if (ReadChars + 8 > FC.size())
-  return;
+  return Error::success();
 
 uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
 ReadChars += 8;
@@ -252,35 +256,35 @@
 
   // Read offset.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read size.
   if (ReadChars + 8 > FC.size())
-return;
+

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133
   /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;

ABataev wrote:
> sdmitriev wrote:
> > sdmitriev wrote:
> > > ABataev wrote:
> > > > Do we really need an inner `Optional` here?
> > > The idea was to return `StringRef` with bundle name when we have still 
> > > have bundles and `None` when there are no more bundles in the file (BTW 
> > > comment has to be updated to describe this). But I can restore the old 
> > > convention where empty bundle name means 'no more bundles' in the file. 
> > > In terms of functionality that would be the same, though use of 
> > > `Optional<...>` makes intentions more explicit))
> > I have updated comment to describe the intended behavior.
> > @ABataev do you insist on removing inner `Optional<>` and restoring the 
> > current convention where empty string means the end of bundles in the file?
> The problem here is that `Expected` already handles the state and we have 
> inner `Optional` for the same reason. Can we reuse `Expected` to indicate 
> that there are no more bundles?
Well, `Expected` encodes two possible states (either Error or Value), but we 
need to encode three states - Error, Value or None. I do not have ideas how to 
do it without adding inner `Optional`. If you have, please share.


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

https://reviews.llvm.org/D67031



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

[PATCH] D69140: [clang-offload-wrapper][NFC] Use captured name of the entry type in LIT test

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6caada4eb465: [clang-offload-wrapper][NFC] Use captured name 
of the entry type in LIT test (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69140

Files:
  clang/test/Driver/clang-offload-wrapper.c


Index: clang/test/Driver/clang-offload-wrapper.c
===
--- clang/test/Driver/clang-offload-wrapper.c
+++ clang/test/Driver/clang-offload-wrapper.c
@@ -31,7 +31,7 @@
 // CHECK-IR: [[ENTBEGIN:@.+]] = external hidden constant [[ENTTY]]
 // CHECK-IR: [[ENTEND:@.+]] = external hidden constant [[ENTTY]]
 
-// CHECK-IR: [[DUMMY:@.+]] = hidden constant [0 x %__tgt_offload_entry] 
zeroinitializer, section "omp_offloading_entries"
+// CHECK-IR: [[DUMMY:@.+]] = hidden constant [0 x [[ENTTY]]] zeroinitializer, 
section "omp_offloading_entries"
 
 // CHECK-IR: [[BIN:@.+]] = internal unnamed_addr constant [[BINTY:\[[0-9]+ x 
i8\]]] c"Content of device file{{.+}}"
 


Index: clang/test/Driver/clang-offload-wrapper.c
===
--- clang/test/Driver/clang-offload-wrapper.c
+++ clang/test/Driver/clang-offload-wrapper.c
@@ -31,7 +31,7 @@
 // CHECK-IR: [[ENTBEGIN:@.+]] = external hidden constant [[ENTTY]]
 // CHECK-IR: [[ENTEND:@.+]] = external hidden constant [[ENTTY]]
 
-// CHECK-IR: [[DUMMY:@.+]] = hidden constant [0 x %__tgt_offload_entry] zeroinitializer, section "omp_offloading_entries"
+// CHECK-IR: [[DUMMY:@.+]] = hidden constant [0 x [[ENTTY]]] zeroinitializer, section "omp_offloading_entries"
 
 // CHECK-IR: [[BIN:@.+]] = internal unnamed_addr constant [[BINTY:\[[0-9]+ x i8\]]] c"Content of device file{{.+}}"
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69140: [clang-offload-wrapper][NFC] Use captured name of the entry type in LIT test

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69140

Files:
  clang/test/Driver/clang-offload-wrapper.c


Index: clang/test/Driver/clang-offload-wrapper.c
===
--- clang/test/Driver/clang-offload-wrapper.c
+++ clang/test/Driver/clang-offload-wrapper.c
@@ -31,7 +31,7 @@
 // CHECK-IR: [[ENTBEGIN:@.+]] = external hidden constant [[ENTTY]]
 // CHECK-IR: [[ENTEND:@.+]] = external hidden constant [[ENTTY]]
 
-// CHECK-IR: [[DUMMY:@.+]] = hidden constant [0 x %__tgt_offload_entry] 
zeroinitializer, section "omp_offloading_entries"
+// CHECK-IR: [[DUMMY:@.+]] = hidden constant [0 x [[ENTTY]]] zeroinitializer, 
section "omp_offloading_entries"
 
 // CHECK-IR: [[BIN:@.+]] = internal unnamed_addr constant [[BINTY:\[[0-9]+ x 
i8\]]] c"Content of device file{{.+}}"
 


Index: clang/test/Driver/clang-offload-wrapper.c
===
--- clang/test/Driver/clang-offload-wrapper.c
+++ clang/test/Driver/clang-offload-wrapper.c
@@ -31,7 +31,7 @@
 // CHECK-IR: [[ENTBEGIN:@.+]] = external hidden constant [[ENTTY]]
 // CHECK-IR: [[ENTEND:@.+]] = external hidden constant [[ENTTY]]
 
-// CHECK-IR: [[DUMMY:@.+]] = hidden constant [0 x %__tgt_offload_entry] zeroinitializer, section "omp_offloading_entries"
+// CHECK-IR: [[DUMMY:@.+]] = hidden constant [0 x [[ENTTY]]] zeroinitializer, section "omp_offloading_entries"
 
 // CHECK-IR: [[BIN:@.+]] = internal unnamed_addr constant [[BINTY:\[[0-9]+ x i8\]]] c"Content of device file{{.+}}"
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev abandoned this revision.
sdmitriev added a comment.

All three parts have been committed, so I am abandoning the original patch.


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

https://reviews.llvm.org/D64943



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

[PATCH] D68931: [clang] [clang-offload-bundler] Fix finding installed llvm-objcopy

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev accepted this revision.
sdmitriev added a comment.

Looks good.


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

https://reviews.llvm.org/D68931



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133
   /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;

sdmitriev wrote:
> ABataev wrote:
> > Do we really need an inner `Optional` here?
> The idea was to return `StringRef` with bundle name when we have still have 
> bundles and `None` when there are no more bundles in the file (BTW comment 
> has to be updated to describe this). But I can restore the old convention 
> where empty bundle name means 'no more bundles' in the file. In terms of 
> functionality that would be the same, though use of `Optional<...>` makes 
> intentions more explicit))
I have updated comment to describe the intended behavior.
@ABataev do you insist on removing inner `Optional<>` and restoring the current 
convention where empty string means the end of bundles in the file?


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 224726.
sdmitriev added a comment.

Rebased patch.


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

https://reviews.llvm.org/D67031

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -26,15 +26,17 @@
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include 
 #include 
 #include 
@@ -42,6 +44,7 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace llvm;
 using namespace llvm::object;
@@ -123,36 +126,37 @@
   virtual ~FileHandler() {}
 
   /// Update the file handler with information from the header of the bundled
-  /// file
-  virtual void ReadHeader(MemoryBuffer ) = 0;
+  /// file.
+  virtual Error ReadHeader(MemoryBuffer ) = 0;
 
-  /// Read the marker of the next bundled to be read in the file. The triple of
-  /// the target associated with that bundle is returned. An empty string is
-  /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  /// Read the marker of the next bundled to be read in the file. The bundle
+  /// name is returned if there is one in the file, or `None` if there are no
+  /// more bundles to be read.
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 
   /// Write the header of the bundled file to \a OS based on the information
   /// gathered from \a Inputs.
-  virtual void WriteHeader(raw_fd_ostream ,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
 
   /// Write the marker that closes a bundle for the triple \a TargetTriple to \a
-  /// OS. Return true if any error was found.
-
-  virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -224,7 +228,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer ) final {
+  Error ReadHeader(MemoryBuffer ) final {
 StringRef FC = Input.getBuffer();
 
 // Initialize the current bundle with the end of the container.
@@ -233,16 +237,16 @@
 // Check if buffer is smaller than magic string.
 size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
 if (ReadChars > FC.size())
-  return;
+  return Error::success();
 
 // Check if no magic was found.
 StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
 if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
-  return;
+  return Error::success();
 
 // Read number of bundles.
 if (ReadChars + 8 > FC.size())
-  return;
+  return Error::success();
 
 uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
 ReadChars += 8;
@@ -252,35 +256,35 @@
 
   // Read offset.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read size.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Size = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read triple size.
   if (ReadChars + 8 > FC.size())
-return;
+  

[PATCH] D68746: [Clang][OpenMP Offload] Move offload registration code to the wrapper

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:74
+  IntegerType *getSizeTTy() {
+switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) {
+case 4u:

grokos wrote:
> sdmitriev wrote:
> > ABataev wrote:
> > > sdmitriev wrote:
> > > > ABataev wrote:
> > > > > Same question as before: maybe better to make the size of size_t type 
> > > > > a parameter of a tool?
> > > > As I remember you also had another suggestion - change size_t to 
> > > > intptr_t. That will eliminate the need to an additional parameter for 
> > > > size type. Will it be better?
> > > In thi case we'll need to change the type in the libomptarget.
> > Right. @grokos , do you see any potential problems in changing 
> > __tgt_offload_entry::size type from size_t to intptr_t?
> As long as `intptr_t` has the same size as `size_t` it should be fine. Of 
> course, if this is not the case, then if libomptarget tries to load an older 
> image where `sizeof(__tgt_offload_entry::size) != sizeof(intptr_t)` then 
> backwards compatibility will have been broken. Fortunately, on all platforms 
> supported by released versions of libomptarget so far (x86_64, ppc64, 
> aarch64) if I'm not mistaken `sizeof(size_t) == sizeof(initptr_t)`, so I 
> don't think we'll break anything.
@ABataev, as I understand clang CG assumes that intptr_t, size_t, and ptrdiff_t 
are the same types.
Please look at clang/lib/CodeGen/CodeGenTypeCache.h, lines 44-49

```
  /// intptr_t, size_t, and ptrdiff_t, which we assume are the same size.
  union {
llvm::IntegerType *IntPtrTy;
llvm::IntegerType *SizeTy;
llvm::IntegerType *PtrDiffTy;
  };
```
So, I guess what we have here is Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68746



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

[PATCH] D68746: [Clang][OpenMP Offload] Move offload registration code to the wrapper

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a subscriber: grokos.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:74
+  IntegerType *getSizeTTy() {
+switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) {
+case 4u:

ABataev wrote:
> sdmitriev wrote:
> > ABataev wrote:
> > > Same question as before: maybe better to make the size of size_t type a 
> > > parameter of a tool?
> > As I remember you also had another suggestion - change size_t to intptr_t. 
> > That will eliminate the need to an additional parameter for size type. Will 
> > it be better?
> In thi case we'll need to change the type in the libomptarget.
Right. @grokos , do you see any potential problems in changing 
__tgt_offload_entry::size type from size_t to intptr_t?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68746



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

[PATCH] D68746: [Clang][OpenMP Offload] Move offload registration code to the wrapper

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:174
+  /// Global variable that represents BinDesc is returned.
+  GlobalVariable *createBinDesc(ArrayRef> Bufs) {
+// Create external begin/end symbols for the offload entries table.

ABataev wrote:
> Why `ArrayRef`, not `StringRef`? It has a constructor for 
> pointer/length pair.
Well, in this context StringRef type would be similar to ArrayRef, but 
with extra operations for string manipulations. Since we know that device 
images are not strings, we do not really need any string operations for image 
buffers and therefore I think ArrayRef is a better choice here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68746



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

[PATCH] D68746: [Clang][OpenMP Offload] Move offload registration code to the wrapper

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:74
+  IntegerType *getSizeTTy() {
+switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) {
+case 4u:

ABataev wrote:
> Same question as before: maybe better to make the size of size_t type a 
> parameter of a tool?
As I remember you also had another suggestion - change size_t to intptr_t. That 
will eliminate the need to an additional parameter for size type. Will it be 
better?



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:204
+ImagesInits.reserve(Bufs.size());
+for (const ArrayRef  : Bufs) {
+  auto *Data = ConstantDataArray::get(C, Buf);

ABataev wrote:
> No need for `const` and `&` here.
Right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68746



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

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D68166#1702487 , @thakis wrote:

> Out of interest (or ignorance :) ), why is this a separate binary instead of 
> just part of the normal `clang` driver? C, C++, Objective-C, and assembly all 
> can do with a single driver, yet the offload stuff now has both 
> clang-offload-wrapper and clang-offload-bundler. Why isn't just `clang` 
> enough?


Well, theoretically both bunder and wrapper functionality can be implemented 
directly in the clang driver, and so technically these tools can be eliminated. 
But it is just a matter of splitting functionality into well-defined logical 
pieces:))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68166



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

I have uploaded the last part to https://reviews.llvm.org/D68746


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

https://reviews.llvm.org/D64943



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

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa0d83768f108: [Clang][OpenMP Offload] Add new tool for 
wrapping offload device binaries (authored by sdmitriev).

Changed prior to commit:
  https://reviews.llvm.org/D68166?vs=223335=224146#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68166

Files:
  clang/include/clang/Driver/Action.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Action.cpp
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Clang.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/clang-offload-wrapper.c
  clang/test/Driver/openmp-offload-gpu.c
  clang/test/Driver/openmp-offload.c
  clang/tools/CMakeLists.txt
  clang/tools/clang-offload-wrapper/CMakeLists.txt
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp

Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- /dev/null
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -0,0 +1,196 @@
+//===-- clang-offload-wrapper/ClangOffloadWrapper.cpp ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// Implementation of the offload wrapper tool. It takes offload target binaries
+/// as input and creates wrapper bitcode file containing target binaries
+/// packaged as data.
+///
+//===--===//
+
+#include "clang/Basic/Version.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Bitcode/BitcodeWriter.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+
+using namespace llvm;
+
+static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
+
+// Mark all our options with this category, everything else (except for -version
+// and -help) will be hidden.
+static cl::OptionCategory
+ClangOffloadWrapperCategory("clang-offload-wrapper options");
+
+static cl::opt Output("o", cl::Required,
+   cl::desc("Output filename"),
+   cl::value_desc("filename"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+static cl::list Inputs(cl::Positional, cl::OneOrMore,
+cl::desc(""),
+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));
+
+static cl::list
+OffloadTargets("offload-targets", cl::CommaSeparated, cl::OneOrMore,
+   cl::desc("Comma-separated list of device target triples"),
+   cl::value_desc("triples"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+namespace {
+
+class BinaryWrapper {
+public:
+  // Binary descriptor. The first field is the a reference to the binary bits,
+  // and the second is the target triple the binary was built for.
+  using BinaryDesc = std::pair, StringRef>;
+
+private:
+  LLVMContext C;
+  Module M;
+
+  // Saver for generated strings.
+  BumpPtrAllocator Alloc;
+  UniqueStringSaver SS;
+
+private:
+  void createImages(ArrayRef Binaries) {
+for (const BinaryDesc  : Binaries) {
+  StringRef SectionName = SS.save(".omp_offloading." + Bin.second);
+
+  auto *DataC = ConstantDataArray::get(C, Bin.first);
+  auto *ImageB =
+  new GlobalVariable(M, DataC->getType(), /*isConstant=*/true,
+ GlobalVariable::ExternalLinkage, DataC,
+ ".omp_offloading.img_start." + Bin.second);
+  ImageB->setSection(SectionName);
+  ImageB->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  ImageB->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
+  auto *EmptyC =
+  

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 223335.
sdmitriev added a comment.

Rebased patch and changed clang-offload-wrapper CMakeLists.txt to use 
add_clang_tool() rather than add_clang_executable() with a custom install rule.


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

https://reviews.llvm.org/D68166

Files:
  clang/include/clang/Driver/Action.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Action.cpp
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Clang.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/clang-offload-wrapper.c
  clang/test/Driver/openmp-offload-gpu.c
  clang/test/Driver/openmp-offload.c
  clang/tools/CMakeLists.txt
  clang/tools/clang-offload-wrapper/CMakeLists.txt
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp

Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- /dev/null
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -0,0 +1,196 @@
+//===-- clang-offload-wrapper/ClangOffloadWrapper.cpp ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// Implementation of the offload wrapper tool. It takes offload target binaries
+/// as input and creates wrapper bitcode file containing target binaries
+/// packaged as data.
+///
+//===--===//
+
+#include "clang/Basic/Version.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Bitcode/BitcodeWriter.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+
+using namespace llvm;
+
+static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
+
+// Mark all our options with this category, everything else (except for -version
+// and -help) will be hidden.
+static cl::OptionCategory
+ClangOffloadWrapperCategory("clang-offload-wrapper options");
+
+static cl::opt Output("o", cl::Required,
+   cl::desc("Output filename"),
+   cl::value_desc("filename"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+static cl::list Inputs(cl::Positional, cl::OneOrMore,
+cl::desc(""),
+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));
+
+static cl::list
+OffloadTargets("offload-targets", cl::CommaSeparated, cl::OneOrMore,
+   cl::desc("Comma-separated list of device target triples"),
+   cl::value_desc("triples"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+namespace {
+
+class BinaryWrapper {
+public:
+  // Binary descriptor. The first field is the a reference to the binary bits,
+  // and the second is the target triple the binary was built for.
+  using BinaryDesc = std::pair, StringRef>;
+
+private:
+  LLVMContext C;
+  Module M;
+
+  // Saver for generated strings.
+  BumpPtrAllocator Alloc;
+  UniqueStringSaver SS;
+
+private:
+  void createImages(ArrayRef Binaries) {
+for (const BinaryDesc  : Binaries) {
+  StringRef SectionName = SS.save(".omp_offloading." + Bin.second);
+
+  auto *DataC = ConstantDataArray::get(C, Bin.first);
+  auto *ImageB =
+  new GlobalVariable(M, DataC->getType(), /*isConstant=*/true,
+ GlobalVariable::ExternalLinkage, DataC,
+ ".omp_offloading.img_start." + Bin.second);
+  ImageB->setSection(SectionName);
+  ImageB->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  ImageB->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
+  auto *EmptyC =
+  ConstantAggregateZero::get(ArrayType::get(Type::getInt8Ty(C), 0u));
+  auto *ImageE =
+  new GlobalVariable(M, EmptyC->getType(), 

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D68166#1692071 , @JonChesterfield 
wrote:

> I think this patch is a behaviour change. Currently, the target binary is 
> embedded in the host binary at link time. With this change, the contents of 
> the binary are embedded in bitcode which is subsequently fed into the link. 
> If indeed so, that seems strictly better - code in the host that cares about 
> the size of the bitcode now has it available at opt time, instead of at link 
> time. The target specific nastiness objcopy would introduce is neatly 
> sidestepped.
>
> This change takes N binaries (that I think need to be for different triples, 
> or the loop doesn't work) and puts them in separate section-annotated bitcode 
> arrays. Equivalent behaviour would result from calling the tool once per 
> binary and passing the N results onward, e.g. to llvm-link.
>
> The functionality of 'take a binary and embed it in bitcode as a const array' 
> is likely to be useful outside of openmp. I've done similar things in the 
> past in non-portable fashion. Aside from the section and symbol names, I 
> don't think there's anything specific to openmp in the tool.
>
> How would you feel about simplifying the tool to work on one file at a time, 
> with an interface that takes the host target (could default to whatever is 
> running the tool) and a string for section name, which generates some bitcode 
> containing that file as a const array plus start/end symbols derived from the 
> section name? The change would involve deleting the multiple file handling 
> and renaming OffloadTargets to SectionName or similar.
>
> clang-offload-wrapper than becomes binary-to-bitcode-embedder (or better, 
> names are hard), with the intent that projects outside of the openmp target 
> offload compiler could use it.
>
> edit: Or keep the multiple file handling if you prefer, preferably raising an 
> error if there are duplicates in the requested section names


The tool indeed does not have anything specific to OpenMP at this step, but 
that will change in the 3rd part of the D64943 
 where I am planning to move offload 
registration code generation from clang to the wrapper tool. So it will have 
OpenMP specifics in future, though I do not see any problems with enabling it 
for other offloading models. We can always change driver to pass an additional 
information that represent 'offload kind' to the wrapper tool (can for example 
be done in a similar way how it is passed to the bundler tool), and wrapper 
will customize output bit-code depending on the offloading model if there would 
be a need for that.

Regarding the multiple vs single file handling. Wrapping each device binary 
independently would still be possible with multi-file wrapping support, but it 
will just increase startup time without adding any benefits in return (once we 
move offload registration code to the wrapper). So, I think that for OpenMP it 
does not make sense to do it (I cannot say anything about the other offloading 
models though).

Anyway, I suggest so start with something that eliminates OpenMP linker script. 
We can always customize/improve tools in future once there would be a need for 
that.




Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:84
+  void createImages(ArrayRef Binaries) {
+for (const BinaryDesc  : Binaries) {
+  StringRef SectionName = SS.save(".omp_offloading." + Bin.second);

JonChesterfield wrote:
> I don't think this works for multiple binaries with the same target triple. 
> They'll all be put in the same section and there will be duplicate symbols 
> for start/end.
Adding the same target triple to the list of OpenMP targets more than once is 
not supported, so such use case isn't viable:

```
bash-4.2$ clang -fopenmp 
-fopenmp-targets=x86_64-pc-linux-gnu,x86_64-pc-linux-gnu test.c
clang-10: warning: The OpenMP offloading target 'x86_64-pc-linux-gnu' is 
similar to target 'x86_64-pc-linux-gnu' already specified - will be ignored. 
[-Wopenmp-target]
bash-4.2$ 
```

But in any case I am going to remove the code which passes offload target 
triples to the wrapper tool in the last part of D64943 because they will not be 
needed for creating wrapper bit-code. As you know start/end symbols are 
referenced from the offload registration code only, so, moving offload 
registration code to the wrapper bit-code eliminates the need to create global 
start/end symbols with predefined names derived from the triple.


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

https://reviews.llvm.org/D68166



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

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 222917.
sdmitriev marked 2 inline comments as done.
sdmitriev added a comment.

Addressed some comments and rebased patch.


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

https://reviews.llvm.org/D68166

Files:
  clang/include/clang/Driver/Action.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Action.cpp
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Clang.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/clang-offload-wrapper.c
  clang/test/Driver/openmp-offload-gpu.c
  clang/test/Driver/openmp-offload.c
  clang/tools/CMakeLists.txt
  clang/tools/clang-offload-wrapper/CMakeLists.txt
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp

Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- /dev/null
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -0,0 +1,196 @@
+//===-- clang-offload-wrapper/ClangOffloadWrapper.cpp ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// Implementation of the offload wrapper tool. It takes offload target binaries
+/// as input and creates wrapper bitcode file containing target binaries
+/// packaged as data.
+///
+//===--===//
+
+#include "clang/Basic/Version.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Bitcode/BitcodeWriter.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+
+using namespace llvm;
+
+static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
+
+// Mark all our options with this category, everything else (except for -version
+// and -help) will be hidden.
+static cl::OptionCategory
+ClangOffloadWrapperCategory("clang-offload-wrapper options");
+
+static cl::opt Output("o", cl::Required,
+   cl::desc("Output filename"),
+   cl::value_desc("filename"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+static cl::list Inputs(cl::Positional, cl::OneOrMore,
+cl::desc(""),
+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));
+
+static cl::list
+OffloadTargets("offload-targets", cl::CommaSeparated, cl::OneOrMore,
+   cl::desc("Comma-separated list of device target triples"),
+   cl::value_desc("triples"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+namespace {
+
+class BinaryWrapper {
+public:
+  // Binary descriptor. The first field is the a reference to the binary bits,
+  // and the second is the target triple the binary was built for.
+  using BinaryDesc = std::pair, StringRef>;
+
+private:
+  LLVMContext C;
+  Module M;
+
+  // Saver for generated strings.
+  BumpPtrAllocator Alloc;
+  UniqueStringSaver SS;
+
+private:
+  void createImages(ArrayRef Binaries) {
+for (const BinaryDesc  : Binaries) {
+  StringRef SectionName = SS.save(".omp_offloading." + Bin.second);
+
+  auto *DataC = ConstantDataArray::get(C, Bin.first);
+  auto *ImageB =
+  new GlobalVariable(M, DataC->getType(), /*isConstant=*/true,
+ GlobalVariable::ExternalLinkage, DataC,
+ ".omp_offloading.img_start." + Bin.second);
+  ImageB->setSection(SectionName);
+  ImageB->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  ImageB->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
+  auto *EmptyC =
+  ConstantAggregateZero::get(ArrayType::get(Type::getInt8Ty(C), 0u));
+  auto *ImageE =
+  new GlobalVariable(M, EmptyC->getType(), /*isConstant=*/true,
+ 

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133
   /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;

ABataev wrote:
> Do we really need an inner `Optional` here?
The idea was to return `StringRef` with bundle name when we have still have 
bundles and `None` when there are no more bundles in the file (BTW comment has 
to be updated to describe this). But I can restore the old convention where 
empty bundle name means 'no more bundles' in the file. In terms of 
functionality that would be the same, though use of `Optional<...>` makes 
intentions more explicit))


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

https://reviews.llvm.org/D67031



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

[PATCH] D68355: [Clang][Driver][NFC] Corrected DeviceActionBuilder methods' comments.

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL373523: [Clang][Driver][NFC] Corrected DeviceActionBuilder 
methods comments. (authored by sdmitriev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D68355?vs=222896=222906#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D68355

Files:
  cfe/trunk/lib/Driver/Driver.cpp


Index: cfe/trunk/lib/Driver/Driver.cpp
===
--- cfe/trunk/lib/Driver/Driver.cpp
+++ cfe/trunk/lib/Driver/Driver.cpp
@@ -2281,12 +2281,10 @@
   return ABRT_Inactive;
 }
 
-/// Append top level actions generated by the builder. Return true if 
errors
-/// were found.
+/// Append top level actions generated by the builder.
 virtual void appendTopLevelActions(ActionList ) {}
 
-/// Append linker actions generated by the builder. Return true if errors
-/// were found.
+/// Append linker actions generated by the builder.
 virtual void appendLinkDependences(OffloadAction::DeviceDependences ) {}
 
 /// Initialize the builder. Return true if any initialization errors are


Index: cfe/trunk/lib/Driver/Driver.cpp
===
--- cfe/trunk/lib/Driver/Driver.cpp
+++ cfe/trunk/lib/Driver/Driver.cpp
@@ -2281,12 +2281,10 @@
   return ABRT_Inactive;
 }
 
-/// Append top level actions generated by the builder. Return true if errors
-/// were found.
+/// Append top level actions generated by the builder.
 virtual void appendTopLevelActions(ActionList ) {}
 
-/// Append linker actions generated by the builder. Return true if errors
-/// were found.
+/// Append linker actions generated by the builder.
 virtual void appendLinkDependences(OffloadAction::DeviceDependences ) {}
 
 /// Initialize the builder. Return true if any initialization errors are
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68355: [Clang][Driver][NFC] Corrected DeviceActionBuilder methods' comments.

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D68355#1691961 , @ABataev wrote:

> Mark the patch as NFC.


I think I have already done that by adding [NFC] to the commit message. Do I 
need to do anything in addition to that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68355



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

[PATCH] D68355: [Clang][Driver][NFC] Corrected DeviceActionBuilder methods' comments.

[Sergey Dmitriev via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68355

Files:
  clang/lib/Driver/Driver.cpp


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2281,12 +2281,10 @@
   return ABRT_Inactive;
 }
 
-/// Append top level actions generated by the builder. Return true if 
errors
-/// were found.
+/// Append top level actions generated by the builder.
 virtual void appendTopLevelActions(ActionList ) {}
 
-/// Append linker actions generated by the builder. Return true if errors
-/// were found.
+/// Append linker actions generated by the builder.
 virtual void appendLinkDependences(OffloadAction::DeviceDependences ) {}
 
 /// Initialize the builder. Return true if any initialization errors are


Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -2281,12 +2281,10 @@
   return ABRT_Inactive;
 }
 
-/// Append top level actions generated by the builder. Return true if errors
-/// were found.
+/// Append top level actions generated by the builder.
 virtual void appendTopLevelActions(ActionList ) {}
 
-/// Append linker actions generated by the builder. Return true if errors
-/// were found.
+/// Append linker actions generated by the builder.
 virtual void appendLinkDependences(OffloadAction::DeviceDependences ) {}
 
 /// Initialize the builder. Return true if any initialization errors are
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:2284-2289
-/// Append top level actions generated by the builder. Return true if 
errors
-/// were found.
+/// Append top level actions generated by the builder.
 virtual void appendTopLevelActions(ActionList ) {}
 
-/// Append linker actions generated by the builder. Return true if errors
-/// were found.

ABataev wrote:
> Could you fix these comments is a separate patch?
Sure. I will prepare a separate patch for the comment change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68166



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

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68166



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Ping.


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

https://reviews.llvm.org/D67031



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

The second part was uploaded to https://reviews.llvm.org/D68166.


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

https://reviews.llvm.org/D64943



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

I have uploaded the first part to https://reviews.llvm.org/D68070


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

https://reviews.llvm.org/D64943



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

[PATCH] D65130: [clang][OpenMP] Add clang-offload-wrapper tool

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev abandoned this revision.
sdmitriev added a comment.

This patch is no longer relevant since it was agreed to split 
https://reviews.llvm.org/D64943 into pieces in a different way.


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

https://reviews.llvm.org/D65130



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Ping.


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Ping.


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

https://reviews.llvm.org/D67031



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D64943#1668006 , @JonChesterfield 
wrote:

> >> Does this diff mix getting rid of the linker script with other changes? 
> >> E.g. it looks like the metadata generation is moving from clang to the new 
> >> tool, but that seems orthogonal to dropping the linker script.
> > 
> > Metadata is still generated by the clang, there are no changes in this 
> > area. What is moving to a wrapper tool is the generation of the offload 
> > registration code. Let me just attach the slides that I presented on the 
> > inter company meeting were the proposal was discussed. It'll probably 
> > answer most of your questions. F9983224: openmp_linker_script.pdf 
> > 
>
> It does indeed, thanks. I see the motivation for delaying offload 
> registration code. I'm pretty sure that is indeed orthogonal to removing the 
> linker script.
>
> How would you feel about using objcopy to embed the device binary?


I see some problems with using llvm-objcopy for that. First issue is that 
symbols created by llvm-objcopy for embedded data depend on the input file 
name. As you know these symbols are referenced from the offload registration 
code that is currently added to an object by the clang at compile time. I not 
sure how you can guarantee that symbol names will match. And another, more 
important problem is that it won't work on Windows because llvm-objcopy 
produces ELF object according to the description.

Anyway I am going to change entries section name to "omp_offloading_entries", 
remove omptargetbegin.o/omptargetend.o and upload the revised patch.


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

https://reviews.llvm.org/D64943



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D64943#1667469 , @JonChesterfield 
wrote:

> I'm on board with getting rid of the linker script. Gold's limited support 
> for that seems conclusive.
>
> I believe the current script does two things:
>  1/ takes a binary and embeds it in a section named 
> .omp_offloading.amdgcn-amd-amdhsa
>  2/ provides start/end symbols for that section and for 
> .omp_offloading.entries.
>
> 2/ is discussed above.
>  1/ can be implemented as a call to (llvm-)objcopy
>
> > If binary is used as the value for --input-target, the input file will be 
> > embedded as a data section in an ELF relocatable object, with symbols 
> > _binary__start, _binary__end, and 
> > _binary__size representing the start, end and size of the data, 
> > where  is the path of the input file as specified on the command 
> > line with non-alphanumeric characters converted to _.
>
> I think dropping the linker script means that cmake will need to invoke an 
> extra executable. As far as I can see, that tool can be objcopy instead of 
> clang-offload-wrapper.
>
> Does this diff mix getting rid of the linker script with other changes? E.g. 
> it looks like the metadata generation is moving from clang to the new tool, 
> but that seems orthogonal to dropping the linker script.


Metadata is still generated by the clang, there are no changes in this area. 
What is moving to a wrapper tool is the generation of the offload registration 
code. Let me just attach the slides that I presented on the inter company 
meeting were the proposal was discussed. It'll probably answer most of your 
questions. F9983224: openmp_linker_script.pdf 



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

https://reviews.llvm.org/D64943



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D64943#1666924 , @JonChesterfield 
wrote:

> In D64943#1666849 , @sdmitriev wrote:
>
> > In D64943#136 , 
> > @JonChesterfield wrote:
> >
> > > I'm not sure copying the crtbegin/crtend mechanism from the early days of 
> > > C runtime is ideal. Since the data is stored in a common section anyway, 
> > > please could we rename it to __omp_offloading_entries in which case the 
> > > linker will provide start/end symbols automatically?
> >
> >
> > Well, I never said that it is an ideal solution, but it is a known 
> > mechanism that works well in many cases and can also be reused for the 
> > offloading entry table.
> >  I do not fully understand your suggestion for renaming entries section, 
> > how it will help with providing start/end symbols for the entries. Can you 
> > please provide more details?
>
>
> Given a custom elf section with a C identifier as a name, the linker will 
> provide definitions of `__start_name`/`__stop_name` to satisfy unresolved 
> symbols. I don't believe this occurs if the section name is not a C 
> identifier, e.g. contains a period. So unless I've misinterpreted the purpose 
> of the two object files, they can be removed in exchange for renaming the 
> section.
>
> - had to Google for the Microsoft equivalent, 
> https://stackoverflow.com/questions/3808053/how-to-get-a-pointer-to-a-binary-section-in-msvc


Hm, I was not aware of this Linux linker feature, thanks a lot for the 
explanation! I see only one problem with using it as a replacement for the 
begin/end objects – it looks like `__start_name`/`__stop_name` symbols are 
created with `default` visibility instead of `hidden`. I guess it will cause 
problems for offload programs that use shared libraries because DSO’s 
`__start_name`/`__stop_name` symbols will be preempted by the executable’s 
symbols and that is not what we want. Is there any way to change this behavior?

As for the Windows support, you are right, 
`__omp_offloading_entries_begin`/`__omp_offloading_entries_end` symbols can be 
defined in the wrapper bit-code file with a help of the sections grouping 
(https://docs.microsoft.com/en-us/windows/win32/Debug/pe-format#grouped-sections-object-only).
 We were going to add this code to the wrapper tool later while adding Windows 
support.


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

https://reviews.llvm.org/D64943



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D64943#136 , @JonChesterfield 
wrote:

> I'm not sure copying the crtbegin/crtend mechanism from the early days of C 
> runtime is ideal. Since the data is stored in a common section anyway, please 
> could we rename it to __omp_offloading_entries in which case the linker will 
> provide start/end symbols automatically?


Well, I never said that it is an ideal solution, but it is a known mechanism 
that works well in many cases and can also be reused for the offloading entry 
table.
I do not fully understand your suggestion for renaming entries section, how it 
will help with providing start/end symbols for the entries. Can you please 
provide more details?


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

https://reviews.llvm.org/D64943



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D64943#193 , @JonChesterfield 
wrote:

> In D64943#179 , @ABataev wrote:
>
> > In D64943#178 , 
> > @JonChesterfield wrote:
> >
> > > In D64943#173 , @ABataev 
> > > wrote:
> > >
> > > > In D64943#158 , 
> > > > @JonChesterfield wrote:
> > > >
> > > > > > OpenMP linker script is known to cause problems for gold and lld 
> > > > > > linkers on Linux and it will also cause problems for Windows 
> > > > > > enabling in future
> > > > >
> > > > > What are the known problems with the linker script? I'm wondering if 
> > > > > they can be resolved without the overhead of introducing a new tool.
> > > >
> > > >
> > > > They just do not support linker script. And, thus, cannot be used for 
> > > > offloading. Only `ld` supports it.
> > >
> > >
> > > In what respect? I've used linker scripts with both gold and lld, and 
> > > both instances of --help text claim to support them. In the case of lld, 
> > > a very complicated script hit a few internal errors, but I believe 
> > > they've all been fixed since.
> >
> >
> > Hmm, I tried it with gold some time ago and it just did not work for me. 
> > The linking failed with diagnostics that some of the commands in the script 
> > are unknown.
>
>
> The problem turns out to be the 'insert before' statement. ld and lld support 
> it, gold does not. According to 
> https://bugzilla.redhat.com/show_bug.cgi?id=927573, the recommended 
> workaround is essentially that implemented in this differential. See also 
> https://sourceware.org/bugzilla/show_bug.cgi?id=15373.


A small example that I presented on the OpenMP multi company meeting earlier:

  bash-4.2$ cat foo.c
  #include 
  
  int main() {
int X = 0;
  
  #pragma omp target map(tofrom: X)
X += 3;
  
printf("X = %d\n", X);
return 0;
  }
  
  bash-4.2$ clang -fopenmp -fopenmp-targets=x86_64-pc-linux-gnu -fuse-ld=gold 
foo.c
  /usr/bin/ld.gold: error: /tmp/a-c699cd.lk:25:8: syntax error, unexpected 
STRING
  /usr/bin/ld.gold: fatal error: unable to parse script file /tmp/a-c699cd.lk
  clang-10: error: linker command failed with exit code 1 (use -v to see 
invocation)
  bash-4.2$ clang -fopenmp -fopenmp-targets=x86_64-pc-linux-gnu -fuse-ld=lld 
foo.c
  ld.lld: error: unable to INSERT AFTER/BEFORE .data: section not defined
  clang-10: error: linker command failed with exit code 1 (use -v to see 
invocation)
  bash-4.2$ 

Also OpenMP linker script will obviously cause problems on Windows once we 
start enabling offload on Windows.


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

https://reviews.llvm.org/D64943



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

[PATCH] D65130: [clang][OpenMP] Add clang-offload-wrapper tool

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 219750.
sdmitriev added a comment.

Rebase


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

https://reviews.llvm.org/D65130

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

Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- /dev/null
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -0,0 +1,360 @@
+//===-- clang-offload-wrapper/ClangOffloadWrapper.cpp ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// Implementation of the offload wrapper tool. It takes offload target binaries
+/// as input and creates wrapper bitcode from them which registers given
+/// binaries in offload runtime.
+///
+//===--===//
+
+#include "clang/Basic/Version.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Bitcode/BitcodeWriter.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
+#include 
+#include 
+
+using namespace llvm;
+
+static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
+
+// Mark all our options with this category, everything else (except for -version
+// and -help) will be hidden.
+static cl::OptionCategory
+ClangOffloadWrapperCategory("clang-offload-wrapper options");
+
+static cl::opt Output("o", cl::Required,
+   cl::desc("Output filename"),
+   cl::value_desc("filename"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+static cl::list Inputs(cl::Positional, cl::OneOrMore,
+cl::desc(""),
+cl::cat(ClangOffloadWrapperCategory));
+
+static cl::opt Target("target", cl::Required,
+   cl::desc("Target triple"),
+   cl::value_desc("triple"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+namespace {
+
+class BinaryWrapper {
+  LLVMContext C;
+  Module M;
+
+  StructType *EntryTy = nullptr;
+  StructType *ImageTy = nullptr;
+  StructType *DescTy = nullptr;
+
+private:
+  IntegerType *getSizeTTy() {
+switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) {
+case 4u:
+  return Type::getInt32Ty(C);
+case 8u:
+  return Type::getInt64Ty(C);
+}
+llvm_unreachable("unsupported pointer type size");
+  }
+
+  // struct __tgt_offload_entry {
+  //   void *addr;
+  //   char *name;
+  //   size_t size;
+  //   int32_t flags;
+  //   int32_t reserved;
+  // };
+  StructType *getEntryTy() {
+if (!EntryTy)
+  EntryTy = StructType::create("__tgt_offload_entry", Type::getInt8PtrTy(C),
+   Type::getInt8PtrTy(C), getSizeTTy(),
+   Type::getInt32Ty(C), Type::getInt32Ty(C));
+return EntryTy;
+  }
+
+  PointerType *getEntryPtrTy() { return PointerType::getUnqual(getEntryTy()); }
+
+  // struct __tgt_device_image {
+  //   void *ImageStart;
+  //   void *ImageEnd;
+  //   __tgt_offload_entry *EntriesBegin;
+  //   __tgt_offload_entry *EntriesEnd;
+  // };
+  StructType *getDeviceImageTy() {
+if (!ImageTy)
+  ImageTy = StructType::create("__tgt_device_image", Type::getInt8PtrTy(C),
+   Type::getInt8PtrTy(C), getEntryPtrTy(),
+   getEntryPtrTy());
+return ImageTy;
+  }
+
+  PointerType *getDeviceImagePtrTy() {
+return PointerType::getUnqual(getDeviceImageTy());
+  }
+
+  // struct __tgt_bin_desc {
+  //   int32_t NumDeviceImages;
+  //   __tgt_device_image *DeviceImages;
+  //   __tgt_offload_entry *HostEntriesBegin;
+  //   __tgt_offload_entry *HostEntriesEnd;
+  // };
+  StructType *getBinDescTy() {
+if (!DescTy)
+  DescTy = StructType::create("__tgt_bin_desc", Type::getInt32Ty(C),
+  getDeviceImagePtrTy(), getEntryPtrTy(),
+  

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 219742.
sdmitriev added a reviewer: Hahnfeld.
sdmitriev added a comment.

Rebase


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

https://reviews.llvm.org/D67031

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -26,15 +26,17 @@
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include 
 #include 
 #include 
@@ -42,6 +44,7 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace llvm;
 using namespace llvm::object;
@@ -122,35 +125,36 @@
 
   /// Update the file handler with information from the header of the bundled
   /// file
-  virtual void ReadHeader(MemoryBuffer ) = 0;
+  virtual Error ReadHeader(MemoryBuffer ) = 0;
 
   /// Read the marker of the next bundled to be read in the file. The triple of
   /// the target associated with that bundle is returned. An empty string is
   /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 
   /// Write the header of the bundled file to \a OS based on the information
   /// gathered from \a Inputs.
-  virtual void WriteHeader(raw_fd_ostream ,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
 
   /// Write the marker that closes a bundle for the triple \a TargetTriple to \a
-  /// OS. Return true if any error was found.
-
-  virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -222,7 +226,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer ) final {
+  Error ReadHeader(MemoryBuffer ) final {
 StringRef FC = Input.getBuffer();
 
 // Initialize the current bundle with the end of the container.
@@ -231,16 +235,16 @@
 // Check if buffer is smaller than magic string.
 size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
 if (ReadChars > FC.size())
-  return;
+  return Error::success();
 
 // Check if no magic was found.
 StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
 if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
-  return;
+  return Error::success();
 
 // Read number of bundles.
 if (ReadChars + 8 > FC.size())
-  return;
+  return Error::success();
 
 uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
 ReadChars += 8;
@@ -250,35 +254,35 @@
 
   // Read offset.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read size.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Size = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read triple size.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t TripleSize = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read triple.
   if (ReadChars + TripleSize > FC.size())
-

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Ping


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

https://reviews.llvm.org/D64943



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Ping


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

I think I have addressed all comments posted so far. Do you have more 
notes/comments/suggestions?


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44
-#include 
-#include 
-#include 
-#include 

Hahnfeld wrote:
> sdmitriev wrote:
> > Hahnfeld wrote:
> > > sdmitriev wrote:
> > > > Hahnfeld wrote:
> > > > > sdmitriev wrote:
> > > > > > Hahnfeld wrote:
> > > > > > > The code still uses (in the order of marked includes)
> > > > > > >  * `std::unique_ptr`
> > > > > > >  * `std::string`
> > > > > > >  * `std::error_code`
> > > > > > >  * `std::vector`,
> > > > > > > so I don't think these lines should be removed. Same goes for 
> > > > > > > `assert` and probably also `cstddef` / `cstdint` (`uint64_t`?)
> > > > > > Right. Restored required system includes.
> > > > > `vector` is still removed which is definitely used. Are you 100% sure 
> > > > > that `algorithm` and `cstddef` are not needed?
> > > > I have replaced all uses of vector with SmallVector.
> > > > 
> > > > > Are you 100% sure that algorithm and cstddef are not needed?
> > > > 
> > > > Ok, I will add algorithm and cstddef back:)
> > > If you want to replace `vector` by `SmallVector`, this must be its own 
> > > revision.
> > Must? Can you show any document/link explaining this?
> > I have no problem with doing that in a separate commit, not a big deal, 
> > just want to see why it has to be done that way.
> This is common practice for reviews: Only one change per patch and split into 
> smaller patches if possible and logically appropriate.
> 
> I can't quote a written requirement, but the Developer Policy section about 
> [[ https://llvm.org/docs/DeveloperPolicy.html#code-reviews | code reviews ]] 
> mentions splitting patches and there's a whole section about [[ 
> https://llvm.org/docs/DeveloperPolicy.html#incremental-development | 
> Incremental Development ]].
I have reverted vector -> SmallVector change.


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 218776.
sdmitriev edited the summary of this revision.

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

https://reviews.llvm.org/D67031

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/Version.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -25,15 +26,17 @@
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include 
 #include 
 #include 
@@ -42,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace llvm;
 using namespace llvm::object;
@@ -122,35 +126,36 @@
 
   /// Update the file handler with information from the header of the bundled
   /// file
-  virtual void ReadHeader(MemoryBuffer ) = 0;
+  virtual Error ReadHeader(MemoryBuffer ) = 0;
 
   /// Read the marker of the next bundled to be read in the file. The triple of
   /// the target associated with that bundle is returned. An empty string is
   /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 
   /// Write the header of the bundled file to \a OS based on the information
   /// gathered from \a Inputs.
-  virtual void WriteHeader(raw_fd_ostream ,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
 
   /// Write the marker that closes a bundle for the triple \a TargetTriple to \a
-  /// OS. Return true if any error was found.
-
-  virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -222,7 +227,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer ) final {
+  Error ReadHeader(MemoryBuffer ) final {
 StringRef FC = Input.getBuffer();
 
 // Initialize the current bundle with the end of the container.
@@ -231,16 +236,16 @@
 // Check if buffer is smaller than magic string.
 size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
 if (ReadChars > FC.size())
-  return;
+  return Error::success();
 
 // Check if no magic was found.
 StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
 if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
-  return;
+  return Error::success();
 
 // Read number of bundles.
 if (ReadChars + 8 > FC.size())
-  return;
+  return Error::success();
 
 uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
 ReadChars += 8;
@@ -250,35 +255,35 @@
 
   // Read offset.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read size.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Size = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read triple size.
   if (ReadChars + 8 > 

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44
-#include 
-#include 
-#include 
-#include 

Hahnfeld wrote:
> sdmitriev wrote:
> > Hahnfeld wrote:
> > > sdmitriev wrote:
> > > > Hahnfeld wrote:
> > > > > The code still uses (in the order of marked includes)
> > > > >  * `std::unique_ptr`
> > > > >  * `std::string`
> > > > >  * `std::error_code`
> > > > >  * `std::vector`,
> > > > > so I don't think these lines should be removed. Same goes for 
> > > > > `assert` and probably also `cstddef` / `cstdint` (`uint64_t`?)
> > > > Right. Restored required system includes.
> > > `vector` is still removed which is definitely used. Are you 100% sure 
> > > that `algorithm` and `cstddef` are not needed?
> > I have replaced all uses of vector with SmallVector.
> > 
> > > Are you 100% sure that algorithm and cstddef are not needed?
> > 
> > Ok, I will add algorithm and cstddef back:)
> If you want to replace `vector` by `SmallVector`, this must be its own 
> revision.
Must? Can you show any document/link explaining this?
I have no problem with doing that in a separate commit, not a big deal, just 
want to see why it has to be done that way.


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 218755.

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

https://reviews.llvm.org/D67031

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/Version.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -25,15 +26,17 @@
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include 
 #include 
 #include 
@@ -41,7 +44,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 using namespace llvm;
 using namespace llvm::object;
@@ -122,35 +125,36 @@
 
   /// Update the file handler with information from the header of the bundled
   /// file
-  virtual void ReadHeader(MemoryBuffer ) = 0;
+  virtual Error ReadHeader(MemoryBuffer ) = 0;
 
   /// Read the marker of the next bundled to be read in the file. The triple of
   /// the target associated with that bundle is returned. An empty string is
   /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 
   /// Write the header of the bundled file to \a OS based on the information
   /// gathered from \a Inputs.
-  virtual void WriteHeader(raw_fd_ostream ,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
 
   /// Write the marker that closes a bundle for the triple \a TargetTriple to \a
-  /// OS. Return true if any error was found.
-
-  virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -222,7 +226,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer ) final {
+  Error ReadHeader(MemoryBuffer ) final {
 StringRef FC = Input.getBuffer();
 
 // Initialize the current bundle with the end of the container.
@@ -231,16 +235,16 @@
 // Check if buffer is smaller than magic string.
 size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
 if (ReadChars > FC.size())
-  return;
+  return Error::success();
 
 // Check if no magic was found.
 StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
 if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
-  return;
+  return Error::success();
 
 // Read number of bundles.
 if (ReadChars + 8 > FC.size())
-  return;
+  return Error::success();
 
 uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
 ReadChars += 8;
@@ -250,35 +254,35 @@
 
   // Read offset.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read size.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Size = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read triple size.
   if (ReadChars + 8 > FC.size())
-return;
+

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44
-#include 
-#include 
-#include 
-#include 

Hahnfeld wrote:
> sdmitriev wrote:
> > Hahnfeld wrote:
> > > The code still uses (in the order of marked includes)
> > >  * `std::unique_ptr`
> > >  * `std::string`
> > >  * `std::error_code`
> > >  * `std::vector`,
> > > so I don't think these lines should be removed. Same goes for `assert` 
> > > and probably also `cstddef` / `cstdint` (`uint64_t`?)
> > Right. Restored required system includes.
> `vector` is still removed which is definitely used. Are you 100% sure that 
> `algorithm` and `cstddef` are not needed?
I have replaced all uses of vector with SmallVector.

> Are you 100% sure that algorithm and cstddef are not needed?

Ok, I will add algorithm and cstddef back:)


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added a comment.

Any comments?


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 218250.
sdmitriev added a comment.

Removed trailing '.' from error messages and added few additional changes for 
better error handling.


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

https://reviews.llvm.org/D67031

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/Version.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -25,23 +26,23 @@
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
-#include 
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include 
-#include 
 #include 
 #include 
 #include 
 #include 
-#include 
+#include 
 
 using namespace llvm;
 using namespace llvm::object;
@@ -122,35 +123,36 @@
 
   /// Update the file handler with information from the header of the bundled
   /// file
-  virtual void ReadHeader(MemoryBuffer ) = 0;
+  virtual Error ReadHeader(MemoryBuffer ) = 0;
 
   /// Read the marker of the next bundled to be read in the file. The triple of
   /// the target associated with that bundle is returned. An empty string is
   /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer ) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 
   /// Write the header of the bundled file to \a OS based on the information
   /// gathered from \a Inputs.
-  virtual void WriteHeader(raw_fd_ostream ,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
 
   /// Write the marker that closes a bundle for the triple \a TargetTriple to \a
-  /// OS. Return true if any error was found.
-
-  virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+  virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -222,7 +224,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer ) final {
+  Error ReadHeader(MemoryBuffer ) final {
 StringRef FC = Input.getBuffer();
 
 // Initialize the current bundle with the end of the container.
@@ -231,16 +233,16 @@
 // Check if buffer is smaller than magic string.
 size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
 if (ReadChars > FC.size())
-  return;
+  return Error::success();
 
 // Check if no magic was found.
 StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
 if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
-  return;
+  return Error::success();
 
 // Read number of bundles.
 if (ReadChars + 8 > FC.size())
-  return;
+  return Error::success();
 
 uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
 ReadChars += 8;
@@ -250,35 +252,35 @@
 
   // Read offset.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
   ReadChars += 8;
 
   // Read size.
   if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
 
   uint64_t Size = Read8byteIntegerFromBuffer(FC, 

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:906
+Msg << ", unknown target triple '" << Triple << "'";
+  reportError(createStringError(errc::invalid_argument, Msg.str() + "."));
 }

MaskRay wrote:
> I think trailing full stop is uncommon in error messages.
Maybe., but all error messages in this tool seem to be consistent in that 
sense)) Do you suggest removing trailing '.' from all error messages?


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44
-#include 
-#include 
-#include 
-#include 

Hahnfeld wrote:
> The code still uses (in the order of marked includes)
>  * `std::unique_ptr`
>  * `std::string`
>  * `std::error_code`
>  * `std::vector`,
> so I don't think these lines should be removed. Same goes for `assert` and 
> probably also `cstddef` / `cstdint` (`uint64_t`?)
Right. Restored required system includes.


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

https://reviews.llvm.org/D67031



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

[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 218224.
sdmitriev retitled this revision from "[Clang][Bundler] Error reporting 
improvements [NFC]" to "[Clang][Bundler] Error reporting improvements".
sdmitriev added a comment.

Addressed review comments.


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

https://reviews.llvm.org/D67031

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/Version.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -25,23 +26,23 @@
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
-#include 
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include 
-#include 
 #include 
 #include 
 #include 
 #include 
-#include 
+#include 
 
 using namespace llvm;
 using namespace llvm::object;
@@ -98,6 +99,9 @@
 /// Path to the current binary.
 static std::string BundlerExecutable;
 
+/// Saved argv[0].
+static StringRef Argv0;
+
 /// Obtain the offload kind and real machine triple out of the target
 /// information specified by the user.
 static void getOffloadKindAndTriple(StringRef Target, StringRef ,
@@ -113,6 +117,15 @@
   return OffloadKind == "host";
 }
 
+/// Error reporting functions.
+static void reportError(Error E) {
+  logAllUnhandledErrors(std::move(E), WithColor::error(errs(), Argv0));
+}
+
+static void reportFileError(StringRef File, std::error_code EC) {
+  reportError(createFileError(File, EC));
+}
+
 /// Generic file handler interface.
 class FileHandler {
 public:
@@ -146,7 +159,6 @@
 
   /// Write the marker that closes a bundle for the triple \a TargetTriple to \a
   /// OS. Return true if any error was found.
-
   virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
@@ -327,7 +339,7 @@
 
 unsigned Idx = 0;
 for (auto  : TargetNames) {
-  MemoryBuffer  = *Inputs[Idx++].get();
+  MemoryBuffer  = *Inputs[Idx++];
   // Bundle offset.
   Write8byteIntegerToBuffer(OS, HeaderSize);
   // Size of the bundle (adds to the next bundle's offset)
@@ -467,7 +479,8 @@
 ErrorOr Objcopy = sys::findProgramByName(
 "llvm-objcopy", sys::path::parent_path(BundlerExecutable));
 if (!Objcopy) {
-  errs() << "error: unable to find 'llvm-objcopy' in path.\n";
+  reportError(createStringError(Objcopy.getError(),
+"unable to find 'llvm-objcopy' in path."));
   return true;
 }
 
@@ -489,13 +502,14 @@
 // If the user asked for the commands to be printed out, we do that instead
 // of executing it.
 if (PrintExternalCommands) {
-  errs() << "\"" << Objcopy.get() << "\"";
+  errs() << "\"" << *Objcopy << "\"";
   for (StringRef Arg : drop_begin(ObjcopyArgs, 1))
 errs() << " \"" << Arg << "\"";
   errs() << "\n";
 } else {
-  if (sys::ExecuteAndWait(Objcopy.get(), ObjcopyArgs)) {
-errs() << "error: llvm-objcopy tool failed.\n";
+  if (sys::ExecuteAndWait(*Objcopy, ObjcopyArgs)) {
+reportError(createStringError(inconvertibleErrorCode(),
+  "'llvm-objcopy' tool failed."));
 return true;
   }
 }
@@ -622,13 +636,11 @@
   // We only support regular object files. If this is not an object file,
   // default to the binary handler. The handler will be owned by the client of
   // this function.
-  std::unique_ptr Obj(
-  dyn_cast(BinaryOrErr.get().release()));
-
-  if (!Obj)
-return new BinaryFileHandler();
-
-  return new ObjectFileHandler(std::move(Obj));
+  if (auto *Obj = dyn_cast(BinaryOrErr->get())) {
+  BinaryOrErr->release();
+  return new ObjectFileHandler(std::unique_ptr(Obj));
+  }
+  return new BinaryFileHandler();
 }
 
 /// Return an appropriate handler given the input files and options.
@@ -650,7 +662,10 @@
   if (FilesType == "ast")
 return new BinaryFileHandler();
 
-  errs() << "error: invalid file type specified.\n";
+  reportError(
+  createStringError(errc::invalid_argument,
+"'" + FilesType + "': 

[PATCH] D67031: [Clang][Bundler] Error reporting improvements [NFC]

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added a reviewer: alexshap.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

https://reviews.llvm.org/D67031

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -25,23 +25,17 @@
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 using namespace llvm::object;
@@ -98,6 +92,9 @@
 /// Path to the current binary.
 static std::string BundlerExecutable;
 
+/// Saved argv[0].
+static StringRef Argv0;
+
 /// Obtain the offload kind and real machine triple out of the target
 /// information specified by the user.
 static void getOffloadKindAndTriple(StringRef Target, StringRef ,
@@ -113,6 +110,15 @@
   return OffloadKind == "host";
 }
 
+/// Error reporting functions.
+static void reportError(Error E) {
+  logAllUnhandledErrors(std::move(E), WithColor::error(errs(), Argv0));
+}
+
+static void reportFileError(StringRef File, std::error_code EC) {
+  reportError(createFileError(File, EC));
+}
+
 /// Generic file handler interface.
 class FileHandler {
 public:
@@ -146,7 +152,6 @@
 
   /// Write the marker that closes a bundle for the triple \a TargetTriple to \a
   /// OS. Return true if any error was found.
-
   virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
@@ -327,7 +332,7 @@
 
 unsigned Idx = 0;
 for (auto  : TargetNames) {
-  MemoryBuffer  = *Inputs[Idx++].get();
+  MemoryBuffer  = *Inputs[Idx++];
   // Bundle offset.
   Write8byteIntegerToBuffer(OS, HeaderSize);
   // Size of the bundle (adds to the next bundle's offset)
@@ -467,7 +472,8 @@
 ErrorOr Objcopy = sys::findProgramByName(
 "llvm-objcopy", sys::path::parent_path(BundlerExecutable));
 if (!Objcopy) {
-  errs() << "error: unable to find 'llvm-objcopy' in path.\n";
+  reportError(createStringError(Objcopy.getError(),
+"unable to find 'llvm-objcopy' in path."));
   return true;
 }
 
@@ -489,13 +495,14 @@
 // If the user asked for the commands to be printed out, we do that instead
 // of executing it.
 if (PrintExternalCommands) {
-  errs() << "\"" << Objcopy.get() << "\"";
+  errs() << "\"" << *Objcopy << "\"";
   for (StringRef Arg : drop_begin(ObjcopyArgs, 1))
 errs() << " \"" << Arg << "\"";
   errs() << "\n";
 } else {
-  if (sys::ExecuteAndWait(Objcopy.get(), ObjcopyArgs)) {
-errs() << "error: llvm-objcopy tool failed.\n";
+  if (sys::ExecuteAndWait(*Objcopy, ObjcopyArgs)) {
+reportError(createStringError(inconvertibleErrorCode(),
+  "'llvm-objcopy' tool failed."));
 return true;
   }
 }
@@ -622,13 +629,11 @@
   // We only support regular object files. If this is not an object file,
   // default to the binary handler. The handler will be owned by the client of
   // this function.
-  std::unique_ptr Obj(
-  dyn_cast(BinaryOrErr.get().release()));
-
-  if (!Obj)
-return new BinaryFileHandler();
-
-  return new ObjectFileHandler(std::move(Obj));
+  if (auto *Obj = dyn_cast(BinaryOrErr->get())) {
+  BinaryOrErr->release();
+  return new ObjectFileHandler(std::unique_ptr(Obj));
+  }
+  return new BinaryFileHandler();
 }
 
 /// Return an appropriate handler given the input files and options.
@@ -650,7 +655,10 @@
   if (FilesType == "ast")
 return new BinaryFileHandler();
 
-  errs() << "error: invalid file type specified.\n";
+  reportError(
+  createStringError(errc::invalid_argument,
+"'" + FilesType + "': invalid file type specified."));
+
   return nullptr;
 }
 
@@ -662,7 +670,7 @@
   raw_fd_ostream OutputFile(OutputFileNames.front(), EC, sys::fs::OF_None);
 
   if (EC) {
-errs() << "error: Can't open file " << OutputFileNames.front() << ".\n";
+reportFileError(OutputFileNames.front(), EC);
 return true;
   }
 
@@ -675,31 +683,31 @@
 

[PATCH] D65130: [clang][OpenMP] Add clang-offload-wrapper tool

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 217682.
sdmitriev added a comment.

Rebased and synced up changes with https://reviews.llvm.org/D64943


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

https://reviews.llvm.org/D65130

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

Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- /dev/null
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -0,0 +1,360 @@
+//===-- clang-offload-wrapper/ClangOffloadWrapper.cpp ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// Implememtation of the offload wrapper tool. It takes offload target binaries
+/// as input and creates wrapper bitcode from them which registers given
+/// binaries in offload runtime.
+///
+//===--===//
+
+#include "clang/Basic/Version.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Bitcode/BitcodeWriter.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
+#include 
+#include 
+
+using namespace llvm;
+
+static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
+
+// Mark all our options with this category, everything else (except for -version
+// and -help) will be hidden.
+static cl::OptionCategory
+ClangOffloadWrapperCategory("clang-offload-wrapper options");
+
+static cl::opt Output("o", cl::Required,
+   cl::desc("Output filename"),
+   cl::value_desc("filename"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+static cl::list Inputs(cl::Positional, cl::OneOrMore,
+cl::desc(""),
+cl::cat(ClangOffloadWrapperCategory));
+
+static cl::opt Target("target", cl::Required,
+   cl::desc("Target triple"),
+   cl::value_desc("triple"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+namespace {
+
+class BinaryWrapper {
+  LLVMContext C;
+  Module M;
+
+  StructType *EntryTy = nullptr;
+  StructType *ImageTy = nullptr;
+  StructType *DescTy = nullptr;
+
+private:
+  IntegerType *getSizeTTy() {
+switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) {
+case 4u:
+  return Type::getInt32Ty(C);
+case 8u:
+  return Type::getInt64Ty(C);
+}
+llvm_unreachable("unsupported pointer type size");
+  }
+
+  // struct __tgt_offload_entry {
+  //   void *addr;
+  //   char *name;
+  //   size_t size;
+  //   int32_t flags;
+  //   int32_t reserved;
+  // };
+  StructType *getEntryTy() {
+if (!EntryTy)
+  EntryTy = StructType::create("__tgt_offload_entry", Type::getInt8PtrTy(C),
+   Type::getInt8PtrTy(C), getSizeTTy(),
+   Type::getInt32Ty(C), Type::getInt32Ty(C));
+return EntryTy;
+  }
+
+  PointerType *getEntryPtrTy() { return PointerType::getUnqual(getEntryTy()); }
+
+  // struct __tgt_device_image {
+  //   void *ImageStart;
+  //   void *ImageEnd;
+  //   __tgt_offload_entry *EntriesBegin;
+  //   __tgt_offload_entry *EntriesEnd;
+  // };
+  StructType *getDeviceImageTy() {
+if (!ImageTy)
+  ImageTy = StructType::create("__tgt_device_image", Type::getInt8PtrTy(C),
+   Type::getInt8PtrTy(C), getEntryPtrTy(),
+   getEntryPtrTy());
+return ImageTy;
+  }
+
+  PointerType *getDeviceImagePtrTy() {
+return PointerType::getUnqual(getDeviceImageTy());
+  }
+
+  // struct __tgt_bin_desc {
+  //   int32_t NumDeviceImages;
+  //   __tgt_device_image *DeviceImages;
+  //   __tgt_offload_entry *HostEntriesBegin;
+  //   __tgt_offload_entry *HostEntriesEnd;
+  // };
+  StructType *getBinDescTy() {
+if (!DescTy)
+  DescTy = StructType::create("__tgt_bin_desc", Type::getInt32Ty(C),
+  getDeviceImagePtrTy(), 

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Looks like there will be no more comments. If so, I will update the first part 
https://reviews.llvm.org/D65130 which adds clang-offload-wrapper tool with the 
latest changes.


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

https://reviews.llvm.org/D64943



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

[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370143: [Clang][Bundler] Do not require host triple for 
extracting device bundles (authored by sdmitriev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66601?vs=217522=217558#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66601

Files:
  cfe/trunk/test/Driver/clang-offload-bundler.c
  cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: cfe/trunk/test/Driver/clang-offload-bundler.c
===
--- cfe/trunk/test/Driver/clang-offload-bundler.c
+++ cfe/trunk/test/Driver/clang-offload-bundler.c
@@ -156,14 +156,20 @@
 // RUN: diff %t.i %t.res.i
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=i -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.i -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: clang-offload-bundler -type=ii -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ii,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ii -unbundle
 // RUN: diff %t.ii %t.res.ii
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=ii -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.ii -unbundle
+// RUN: diff %t.tgt2 %t.res.tgt2
 // RUN: clang-offload-bundler -type=ll -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ll,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ll -unbundle
 // RUN: diff %t.ll %t.res.ll
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=ll -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.ll -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: clang-offload-bundler -type=s -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.s,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.s -unbundle
 // RUN: diff %t.s %t.res.s
 // RUN: diff %t.tgt1 %t.res.tgt1
@@ -172,6 +178,8 @@
 // RUN: diff %t.s %t.res.s
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=s -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt2 %t.res.tgt2
 
 // Check if we can unbundle a file with no magic strings.
 // RUN: clang-offload-bundler -type=s -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.s,%t.res.tgt1,%t.res.tgt2 -inputs=%t.s -unbundle
@@ -183,6 +191,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that bindler prints an error if given host bundle does not exist in the fat binary.
+// RUN: not clang-offload-bundler -type=s -targets=host-x86_64-xxx-linux-gnu,openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.s,%t.res.tgt1 -inputs=%t.bundle3.s -unbundle 2>&1 | FileCheck %s --check-prefix CK-NO-HOST-BUNDLE
+// CK-NO-HOST-BUNDLE: error: Can't find bundle for the host target
+
 //
 // Check binary bundle/unbundle. The content that we have before bundling must be the same we have after unbundling.
 //
@@ -194,10 +206,14 @@
 // RUN: diff %t.bc %t.res.bc
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=bc -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.bc -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: clang-offload-bundler -type=gch -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.gch,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.gch -unbundle
 // RUN: diff %t.ast %t.res.gch
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=gch -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.gch -unbundle
+// RUN: diff %t.tgt2 %t.res.tgt2
 // RUN: clang-offload-bundler -type=ast -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ast,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ast -unbundle
 // RUN: diff %t.ast %t.res.ast
 // RUN: diff %t.tgt1 %t.res.tgt1
@@ -210,6 +226,8 @@
 // RUN: diff %t.ast %t.res.ast
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=ast -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.ast -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
 
 // Check if we can unbundle a file with no magic strings.
 // RUN: clang-offload-bundler -type=bc 

[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:888
+  // treat missing host triple as error if we do unbundling.
+  if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
 Error = true;

ABataev wrote:
> ABataev wrote:
> > sdmitriev wrote:
> > > sdmitriev wrote:
> > > > sdmitriev wrote:
> > > > > ABataev wrote:
> > > > > > sdmitriev wrote:
> > > > > > > ABataev wrote:
> > > > > > > > I believe,  for unbundling we also must check for `!= 1` rather 
> > > > > > > > than `> 1`. Zero host targets also is not allowed.
> > > > > > > But the whole idea of this change is to remove requirement to 
> > > > > > > provide host triple for unbundling operation. Target bundle(s) 
> > > > > > > can always be extracted without extracting host, so host bundle 
> > > > > > > is optional. Therefore zero host targets should not be considered 
> > > > > > > as error for unbundling.
> > > > > > And why do we need this? I think it would be better to check that 
> > > > > > the requested host triple matches the bundled one using this 
> > > > > > parameter rather than removing it.
> > > > > > And why do we need this?
> > > > > 
> > > > > As I wrote in the summary it is a usability issue. You may for 
> > > > > example want to extract device object for a particular offload target 
> > > > > to examine its contents (symbols, sections, etc..), but currently you 
> > > > > also have to extract host bundle as well even if you do not need it.
> > > > > 
> > > > > > I think it would be better to check that the requested host triple 
> > > > > > matches the bundled one using this parameter rather than removing 
> > > > > > it.
> > > > > 
> > > > > So you suggest to check that host bundle name that exists in the fat 
> > > > > image matches the host bundle name provided it command line if it was 
> > > > > provided? Should it be an error if names do not match?
> > > > > 
> > > > I have updated patch to do error checking if host bundle name was 
> > > > provided in command line.
> > > @ABataev I believe the host bundle name is now being checked as you 
> > > suggested. Can you please confirm that it matches your expectations?
> > Ok, I got the idea of the patch. BTW, will happen if I request the device 
> > code for the triple, which is correct, but bundled container does not have 
> > the device code for this triple?
> What about this?
Based on the comments on lines 775-776 and 801 in ClangOffloadBundler.cpp 
bundler will create empty files for missing device bundles.


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

https://reviews.llvm.org/D66601



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

[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 217522.
sdmitriev marked an inline comment as done.
sdmitriev added a comment.

Added tests for each file type.


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

https://reviews.llvm.org/D66601

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -791,8 +791,9 @@
 return false;
   }
 
-  // If we found elements, we emit an error if none of those were for the host.
-  if (!FoundHostBundle) {
+  // If we found elements, we emit an error if none of those were for the host
+  // in case host bundle name was provided in command line.
+  if (!FoundHostBundle && HostInputIndex != ~0u) {
 errs() << "error: Can't find bundle for the host target\n";
 return true;
   }
@@ -895,7 +896,9 @@
 ++Index;
   }
 
-  if (HostTargetNum != 1) {
+  // Host triple is not really needed for unbundling operation, so do not
+  // treat missing host triple as error if we do unbundling.
+  if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
 Error = true;
 errs() << "error: expecting exactly one host target but got "
<< HostTargetNum << ".\n";
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -156,14 +156,20 @@
 // RUN: diff %t.i %t.res.i
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=i -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.i -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: clang-offload-bundler -type=ii -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ii,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ii -unbundle
 // RUN: diff %t.ii %t.res.ii
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=ii -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.ii -unbundle
+// RUN: diff %t.tgt2 %t.res.tgt2
 // RUN: clang-offload-bundler -type=ll -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ll,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.ll -unbundle
 // RUN: diff %t.ll %t.res.ll
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=ll -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.ll -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: clang-offload-bundler -type=s -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.s,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.s -unbundle
 // RUN: diff %t.s %t.res.s
 // RUN: diff %t.tgt1 %t.res.tgt1
@@ -172,6 +178,8 @@
 // RUN: diff %t.s %t.res.s
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=s -targets=openmp-x86_64-pc-linux-gnu -outputs=%t.res.tgt2 -inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt2 %t.res.tgt2
 
 // Check if we can unbundle a file with no magic strings.
 // RUN: clang-offload-bundler -type=s -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.s,%t.res.tgt1,%t.res.tgt2 -inputs=%t.s -unbundle
@@ -183,6 +191,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that bindler prints an error if given host bundle does not exist in the fat binary.
+// RUN: not clang-offload-bundler -type=s -targets=host-x86_64-xxx-linux-gnu,openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.s,%t.res.tgt1 -inputs=%t.bundle3.s -unbundle 2>&1 | FileCheck %s --check-prefix CK-NO-HOST-BUNDLE
+// CK-NO-HOST-BUNDLE: error: Can't find bundle for the host target
+
 //
 // Check binary bundle/unbundle. The content that we have before bundling must be the same we have after unbundling.
 //
@@ -194,10 +206,14 @@
 // RUN: diff %t.bc %t.res.bc
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=bc -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.bc -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: clang-offload-bundler -type=gch -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -outputs=%t.res.gch,%t.res.tgt1,%t.res.tgt2 -inputs=%t.bundle3.gch -unbundle
 // RUN: diff %t.ast %t.res.gch
 // RUN: diff %t.tgt1 %t.res.tgt1
 // RUN: diff %t.tgt2 %t.res.tgt2
+// RUN: clang-offload-bundler -type=gch -targets=openmp-x86_64-pc-linux-gnu 

[PATCH] D66598: [Clang][Bundler] Fix for a hang when unbundling fat binary

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370115: [Clang][Bundler] Fix for a hang when unbundling fat 
binary (authored by sdmitriev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66598?vs=217485=217501#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66598

Files:
  cfe/trunk/test/Driver/clang-offload-bundler.c
  cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: cfe/trunk/test/Driver/clang-offload-bundler.c
===
--- cfe/trunk/test/Driver/clang-offload-bundler.c
+++ cfe/trunk/test/Driver/clang-offload-bundler.c
@@ -221,6 +221,11 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we do not have to unbundle all available bundles from the fat 
binary.
+// RUN: clang-offload-bundler -type=ast 
-targets=host-%itanium_abi_triple,openmp-x86_64-pc-linux-gnu 
-outputs=%t.res.ast,%t.res.tgt2 -inputs=%t.bundle3.unordered.ast -unbundle
+// RUN: diff %t.ast %t.res.ast
+// RUN: diff %t.tgt2 %t.res.tgt2
+
 //
 // Check object bundle/unbundle. The content should be bundled into an ELF
 // section (we are using a PowerPC little-endian host which uses ELF). We
Index: cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -215,6 +215,7 @@
 
   /// Iterator for the bundle information that is being read.
   StringMap::iterator CurBundleInfo;
+  StringMap::iterator NextBundleInfo;
 
 public:
   BinaryFileHandler() : FileHandler() {}
@@ -284,19 +285,19 @@
   BundlesInfo[Triple] = BundleInfo(Size, Offset);
 }
 // Set the iterator to where we will start to read.
-CurBundleInfo = BundlesInfo.begin();
+CurBundleInfo = BundlesInfo.end();
+NextBundleInfo = BundlesInfo.begin();
   }
 
   StringRef ReadBundleStart(MemoryBuffer ) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
   void ReadBundleEnd(MemoryBuffer ) final {
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
-++CurBundleInfo;
   }
 
   void ReadBundle(raw_fd_ostream , MemoryBuffer ) final {


Index: cfe/trunk/test/Driver/clang-offload-bundler.c
===
--- cfe/trunk/test/Driver/clang-offload-bundler.c
+++ cfe/trunk/test/Driver/clang-offload-bundler.c
@@ -221,6 +221,11 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we do not have to unbundle all available bundles from the fat binary.
+// RUN: clang-offload-bundler -type=ast -targets=host-%itanium_abi_triple,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ast,%t.res.tgt2 -inputs=%t.bundle3.unordered.ast -unbundle
+// RUN: diff %t.ast %t.res.ast
+// RUN: diff %t.tgt2 %t.res.tgt2
+
 //
 // Check object bundle/unbundle. The content should be bundled into an ELF
 // section (we are using a PowerPC little-endian host which uses ELF). We
Index: cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ cfe/trunk/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -215,6 +215,7 @@
 
   /// Iterator for the bundle information that is being read.
   StringMap::iterator CurBundleInfo;
+  StringMap::iterator NextBundleInfo;
 
 public:
   BinaryFileHandler() : FileHandler() {}
@@ -284,19 +285,19 @@
   BundlesInfo[Triple] = BundleInfo(Size, Offset);
 }
 // Set the iterator to where we will start to read.
-CurBundleInfo = BundlesInfo.begin();
+CurBundleInfo = BundlesInfo.end();
+NextBundleInfo = BundlesInfo.begin();
   }
 
   StringRef ReadBundleStart(MemoryBuffer ) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
   void ReadBundleEnd(MemoryBuffer ) final {
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
-++CurBundleInfo;
   }
 
   void ReadBundle(raw_fd_ostream , MemoryBuffer ) final {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66598: [Clang][Bundler] Fix for a hang when unbundling fat binary

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:295
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();

ABataev wrote:
> Maybe just:
> ```
> const BundleInfo  = *CurBundleInfo;
> std::advance(CurBundleInfo, 1);
> return Bundle.first();
> ```
> instead of all these changes?
It could be done like this if CurBundleInfo was not used in ReadBundle.


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

https://reviews.llvm.org/D66598



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

[PATCH] D66598: [Clang][Bundler] Fix for a hang when unbundling fat binary

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 217485.

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

https://reviews.llvm.org/D66598

Files:
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -215,6 +215,7 @@
 
   /// Iterator for the bundle information that is being read.
   StringMap::iterator CurBundleInfo;
+  StringMap::iterator NextBundleInfo;
 
 public:
   BinaryFileHandler() : FileHandler() {}
@@ -284,19 +285,19 @@
   BundlesInfo[Triple] = BundleInfo(Size, Offset);
 }
 // Set the iterator to where we will start to read.
-CurBundleInfo = BundlesInfo.begin();
+CurBundleInfo = BundlesInfo.end();
+NextBundleInfo = BundlesInfo.begin();
   }
 
   StringRef ReadBundleStart(MemoryBuffer ) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
   void ReadBundleEnd(MemoryBuffer ) final {
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
-++CurBundleInfo;
   }
 
   void ReadBundle(raw_fd_ostream , MemoryBuffer ) final {
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -221,6 +221,11 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we do not have to unbundle all available bundles from the fat 
binary.
+// RUN: clang-offload-bundler -type=ast 
-targets=host-%itanium_abi_triple,openmp-x86_64-pc-linux-gnu 
-outputs=%t.res.ast,%t.res.tgt2 -inputs=%t.bundle3.unordered.ast -unbundle
+// RUN: diff %t.ast %t.res.ast
+// RUN: diff %t.tgt2 %t.res.tgt2
+
 //
 // Check object bundle/unbundle. The content should be bundled into an ELF
 // section (we are using a PowerPC little-endian host which uses ELF). We


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -215,6 +215,7 @@
 
   /// Iterator for the bundle information that is being read.
   StringMap::iterator CurBundleInfo;
+  StringMap::iterator NextBundleInfo;
 
 public:
   BinaryFileHandler() : FileHandler() {}
@@ -284,19 +285,19 @@
   BundlesInfo[Triple] = BundleInfo(Size, Offset);
 }
 // Set the iterator to where we will start to read.
-CurBundleInfo = BundlesInfo.begin();
+CurBundleInfo = BundlesInfo.end();
+NextBundleInfo = BundlesInfo.begin();
   }
 
   StringRef ReadBundleStart(MemoryBuffer ) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
   void ReadBundleEnd(MemoryBuffer ) final {
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
-++CurBundleInfo;
   }
 
   void ReadBundle(raw_fd_ostream , MemoryBuffer ) final {
Index: clang/test/Driver/clang-offload-bundler.c
===
--- clang/test/Driver/clang-offload-bundler.c
+++ clang/test/Driver/clang-offload-bundler.c
@@ -221,6 +221,11 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we do not have to unbundle all available bundles from the fat binary.
+// RUN: clang-offload-bundler -type=ast -targets=host-%itanium_abi_triple,openmp-x86_64-pc-linux-gnu -outputs=%t.res.ast,%t.res.tgt2 -inputs=%t.bundle3.unordered.ast -unbundle
+// RUN: diff %t.ast %t.res.ast
+// RUN: diff %t.tgt2 %t.res.tgt2
+
 //
 // Check object bundle/unbundle. The content should be bundled into an ELF
 // section (we are using a PowerPC little-endian host which uses ELF). We
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66598: [Clang][Bundler] Fix for a hang when unbundling fat binary

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:301
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
 ++CurBundleInfo;
   }

Just noticed that this line now looks redundant and should be removed.


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

https://reviews.llvm.org/D66598



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

[PATCH] D66598: [Clang][Bundler] Fix for a hang when unbundling fat binary

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Sure. The main unbundling loop looks as follows (see loop on line 745)

  while (!Worklist.empty()) {
StringRef CurTriple = FH.get()->ReadBundleStart(Input);
  
if (CurTriple.empty())
  break;
  
auto Output = Worklist.find(CurTriple);
if (Output == Worklist.end()) {
  continue;
} 
   ...
  }

Here `Worklist` is a collection of bundles that need to be extracted, and `FH` 
is the object implementing `FileHandler` interface. 
`FileHandler::ReadBundleStart()` returns the name of the bundle `FH` currently 
points to. As you can see in the loop above if the name returned by that call 
is not in the set of bundles that need to be extracted, we just call 
`ReadBundleStart` next time assuming that `FileHandler` would advance to the 
next bundle on each `ReadBundleStart` call. That assumption is correct for all 
`FileHandler` implementations except `BinaryFileHandler` which does not advance 
to the next bundle when this method is called. As a result we are going into an 
infinite loop if we need to skip a bundle for the file handled by the 
`BinaryFileHandler` . This patch just fixes this problem in the 
`BinaryFileHandler` implementation.


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

https://reviews.llvm.org/D66598



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

[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/test/Driver/clang-offload-bundler.c:228-231
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=ast 
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 
-inputs=%t.bundle3.unordered.ast -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+

sdmitriev wrote:
> ABataev wrote:
> > What about tests for other kinds of elements like preprocessed code, IR, 
> > objects, etc.?
> Ok, I can add more tests for reprocessed code, IR, etc. I have added one test 
> per file handler type so far, but I can do it per file type. BTW, test for 
> object type is already there on line 264.
One note that adding more tests for binary file handler would most probably 
require this fix https://reviews.llvm.org/D66598. Can you please take a look at 
this patch?


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

https://reviews.llvm.org/D66601



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

[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/test/Driver/clang-offload-bundler.c:228-231
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=ast 
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 
-inputs=%t.bundle3.unordered.ast -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+

ABataev wrote:
> What about tests for other kinds of elements like preprocessed code, IR, 
> objects, etc.?
Ok, I can add more tests for reprocessed code, IR, etc. I have added one test 
per file handler type so far, but I can do it per file type. BTW, test for 
object type is already there on line 264.


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

https://reviews.llvm.org/D66601



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

[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:888
+  // treat missing host triple as error if we do unbundling.
+  if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
 Error = true;

sdmitriev wrote:
> sdmitriev wrote:
> > ABataev wrote:
> > > sdmitriev wrote:
> > > > ABataev wrote:
> > > > > I believe,  for unbundling we also must check for `!= 1` rather than 
> > > > > `> 1`. Zero host targets also is not allowed.
> > > > But the whole idea of this change is to remove requirement to provide 
> > > > host triple for unbundling operation. Target bundle(s) can always be 
> > > > extracted without extracting host, so host bundle is optional. 
> > > > Therefore zero host targets should not be considered as error for 
> > > > unbundling.
> > > And why do we need this? I think it would be better to check that the 
> > > requested host triple matches the bundled one using this parameter rather 
> > > than removing it.
> > > And why do we need this?
> > 
> > As I wrote in the summary it is a usability issue. You may for example want 
> > to extract device object for a particular offload target to examine its 
> > contents (symbols, sections, etc..), but currently you also have to extract 
> > host bundle as well even if you do not need it.
> > 
> > > I think it would be better to check that the requested host triple 
> > > matches the bundled one using this parameter rather than removing it.
> > 
> > So you suggest to check that host bundle name that exists in the fat image 
> > matches the host bundle name provided it command line if it was provided? 
> > Should it be an error if names do not match?
> > 
> I have updated patch to do error checking if host bundle name was provided in 
> command line.
@ABataev I believe the host bundle name is now being checked as you suggested. 
Can you please confirm that it matches your expectations?


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

https://reviews.llvm.org/D66601



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

[PATCH] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:888
+  // treat missing host triple as error if we do unbundling.
+  if ((Unbundle && HostTargetNum > 1) || (!Unbundle && HostTargetNum != 1)) {
 Error = true;

sdmitriev wrote:
> ABataev wrote:
> > sdmitriev wrote:
> > > ABataev wrote:
> > > > I believe,  for unbundling we also must check for `!= 1` rather than `> 
> > > > 1`. Zero host targets also is not allowed.
> > > But the whole idea of this change is to remove requirement to provide 
> > > host triple for unbundling operation. Target bundle(s) can always be 
> > > extracted without extracting host, so host bundle is optional. Therefore 
> > > zero host targets should not be considered as error for unbundling.
> > And why do we need this? I think it would be better to check that the 
> > requested host triple matches the bundled one using this parameter rather 
> > than removing it.
> > And why do we need this?
> 
> As I wrote in the summary it is a usability issue. You may for example want 
> to extract device object for a particular offload target to examine its 
> contents (symbols, sections, etc..), but currently you also have to extract 
> host bundle as well even if you do not need it.
> 
> > I think it would be better to check that the requested host triple matches 
> > the bundled one using this parameter rather than removing it.
> 
> So you suggest to check that host bundle name that exists in the fat image 
> matches the host bundle name provided it command line if it was provided? 
> Should it be an error if names do not match?
> 
I have updated patch to do error checking if host bundle name was provided in 
command line.


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

https://reviews.llvm.org/D66601



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