[PATCH] D50218: [OpenMP] Encode offload target triples into comdat key for offload initialization code

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added reviewers: ABataev, hfinkel.
Herald added subscribers: cfe-commits, mgrang, guansong.

Encoding offload target triples onto comdat group key for offload 
initialization code guarantees that it will be executed once per each unique 
combination of offload targets.


Repository:
  rC Clang

https://reviews.llvm.org/D50218

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp


Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,17 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat key for the registration/unregistration code for
+// this particular combination of offloading targets.
+SmallVector RegFnNameParts;
+RegFnNameParts.push_back("omp_offloading");
+RegFnNameParts.push_back("descriptor_reg");
+for (const auto &Device : Devices)
+  RegFnNameParts.push_back(Device.getTriple());
+std::sort(RegFnNameParts.begin() + 2, RegFnNameParts.end());
+std::string Descriptor = getName(RegFnNameParts);
 RegFn = CGM.CreateGlobalInitOrDestructFunction(FTy, Descriptor, FI);
 CGF.StartFunction(GlobalDecl(), C.VoidTy, RegFn, FI, FunctionArgList());
 CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__tgt_register_lib), Desc);


Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,17 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat key for the registration/unregistration code for
+// this particular combination of offloading targets.
+SmallVector RegFnNameParts;
+RegFnNameParts.push_back("omp_offloading");
+RegFnNameParts.push_back("descriptor_reg");
+for (const auto &Device : Devices)
+  RegFnNameParts.push_back(Device.getTriple());
+std::sort(RegFnNameParts.begin() + 2, RegFnNameParts.end());
+std::string Descriptor = getName(RegFnNameParts);
 RegFn = CGM.CreateGlobalInitOrDestructFunction(FTy, Descriptor, FI);
 CGF.StartFunction(GlobalDecl(), C.VoidTy, RegFn, FI, FunctionArgList());
 CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__tgt_register_lib), Desc);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49510: [OpenMP][Clang] Usability improvements for OpenMP offloading

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev abandoned this revision.
sdmitriev added a comment.

A simpler solution was proposed while discussing an RFC which I submitted a 
while ago per Alexey's request. It would lead to (almost) the same results but 
requires much smaller amount of changes.


Repository:
  rC Clang

https://reviews.llvm.org/D49510



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

[PATCH] D50218: [OpenMP] Encode offload target triples into comdat key for offload initialization code

[Sergey Dmitriev via Phabricator via cfe-commits]

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

Replaced std::sort with llvm::sort. Added a test for offload target 
registration code for two offload targets.


https://reviews.llvm.org/D50218

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  test/OpenMP/openmp_offload_registration.cpp


Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,17 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat key for the registration/unregistration code for
+// this particular combination of offloading targets.
+SmallVector RegFnNameParts;
+RegFnNameParts.push_back("omp_offloading");
+RegFnNameParts.push_back("descriptor_reg");
+for (const auto &Device : Devices)
+  RegFnNameParts.push_back(Device.getTriple());
+llvm::sort(RegFnNameParts.begin() + 2, RegFnNameParts.end());
+std::string Descriptor = getName(RegFnNameParts);
 RegFn = CGM.CreateGlobalInitOrDestructFunction(FTy, Descriptor, FI);
 CGF.StartFunction(GlobalDecl(), C.VoidTy, RegFn, FI, FunctionArgList());
 CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__tgt_register_lib), Desc);
Index: test/OpenMP/openmp_offload_registration.cpp
===
--- test/OpenMP/openmp_offload_registration.cpp
+++ test/OpenMP/openmp_offload_registration.cpp
@@ -0,0 +1,49 @@
+// Test for offload registration code for two targets
+// RUN: %clang_cc1 -verify -fopenmp -x c -triple x86_64-unknown-linux-gnu 
-fopenmp-targets=x86_64-pc-linux-gnu,powerpc64le-ibm-linux-gnu -emit-llvm %s -o 
- | FileCheck %s
+// expected-no-diagnostics
+
+void foo() {
+#pragma omp target
+  {}
+}
+
+// CHECK-DAG: [[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]], i32, i32 }
+// CHECK-DAG: [[DEVTY:%.+]] = type { i8*, i8*, [[ENTTY]]*, [[ENTTY]]* }
+// CHECK-DAG: [[DSCTY:%.+]] = type { i32, [[DEVTY]]*, [[ENTTY]]*, [[ENTTY]]* }
+
+// Comdat key for the offload registration code. Should have sorted offload
+// target triples encoded into the name.
+// CHECK-DAG: 
$[[REGFN:\.omp_offloading\..+\.powerpc64le-ibm-linux-gnu\.x86_64-pc-linux-gnu+]]
 = comdat any
+
+// Check if offloading descriptor is created.
+// CHECK: [[ENTBEGIN:@.+]] = external constant [[ENTTY]]
+// CHECK: [[ENTEND:@.+]] = external constant [[ENTTY]]
+// CHECK: [[DEV1BEGIN:@.+]] = extern_weak constant i8
+// CHECK: [[DEV1END:@.+]] = extern_weak constant i8
+// CHECK: [[DEV2BEGIN:@.+]] = extern_weak constant i8
+// CHECK: [[DEV2END:@.+]] = extern_weak constant i8
+// CHECK: [[IMAGES:@.+]] = internal unnamed_addr constant [2 x [[DEVTY]]] 
[{{.+}} { i8* [[DEV1BEGIN]], i8* [[DEV1END]], [[ENTTY]]* [[ENTBEGIN]], 
[[ENTTY]]* [[ENTEND]] }, {{.+}} { i8* [[DEV2BEGIN]], i8* [[DEV2END]], 
[[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }], comdat($[[REGFN]])
+// CHECK: [[DESC:@.+]] = internal constant [[DSCTY]] { i32 2, [[DEVTY]]* 
getelementptr inbounds ([2 x [[DEVTY]]], [2 x [[DEVTY]]]* [[IMAGES]], i32 0, 
i32 0), [[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }, comdat($[[REGFN]])
+
+// Check target registration is registered as a Ctor.
+// CHECK: appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* 
} { i32 0, void ()* @[[REGFN]], i8* bitcast (void ()* @[[REGFN]] to i8*) }]
+
+// Check presence of foo() and the outlined target region
+// CHECK: define void [[FOO:@.+]]()
+// CHECK: define internal void [[OUTLINEDTARGET:@.+]]() 
+
+// Check registration and unregistration code.
+
+// CHECK: define internal void @[[UNREGFN:.+]](i8*)
+// CHECK-SAME: comdat($[[REGFN]]) {
+// CHECK: call i32 @__tgt_unregister_lib([[DSCTY]]* [[DESC]])
+// CHECK: ret void
+// CHECK: declare i32 @__tgt_unregister_lib([[DSCTY]]*)
+
+// CHECK: define linkonce hidden void @[[REGFN]]()
+// CHECK-SAME: comdat {
+// CHECK: call i32 @__tgt_register_lib([[DSCTY]]* [[DESC]])
+// CHECK: call i32 @__cxa_atexit(void (i8*)* @[[UNREGFN]], i8* bitcast 
([[DSCTY]]* [[DESC]] to i8*),
+// CHECK: ret void
+// CHECK: declare i32 @__tgt_register_lib([[DSCTY]]*)
+


Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,17 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat 

[PATCH] D50218: [OpenMP] Encode offload target triples into comdat key for offload initialization code

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3825
+// this particular combination of offloading targets.
+SmallVector RegFnNameParts;
+RegFnNameParts.push_back("omp_offloading");

ABataev wrote:
> Preallocate the memory for all elements at construction, you know the number 
> of elements.
Agree.


https://reviews.llvm.org/D50218



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

[PATCH] D50218: [OpenMP] Encode offload target triples into comdat key for offload initialization code

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 159058.

https://reviews.llvm.org/D50218

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  test/OpenMP/openmp_offload_registration.cpp


Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,17 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat key for the registration/unregistration code for
+// this particular combination of offloading targets.
+SmallVector RegFnNameParts(Devices.size() + 2U);
+RegFnNameParts[0] = "omp_offloading";
+RegFnNameParts[1] = "descriptor_reg";
+for (size_t I = 0; I < Devices.size(); ++I)
+  RegFnNameParts[I + 2U] = Devices[I].getTriple();
+llvm::sort(RegFnNameParts.begin() + 2, RegFnNameParts.end());
+std::string Descriptor = getName(RegFnNameParts);
 RegFn = CGM.CreateGlobalInitOrDestructFunction(FTy, Descriptor, FI);
 CGF.StartFunction(GlobalDecl(), C.VoidTy, RegFn, FI, FunctionArgList());
 CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__tgt_register_lib), Desc);
Index: test/OpenMP/openmp_offload_registration.cpp
===
--- test/OpenMP/openmp_offload_registration.cpp
+++ test/OpenMP/openmp_offload_registration.cpp
@@ -0,0 +1,49 @@
+// Test for offload registration code for two targets
+// RUN: %clang_cc1 -verify -fopenmp -x c -triple x86_64-unknown-linux-gnu 
-fopenmp-targets=x86_64-pc-linux-gnu,powerpc64le-ibm-linux-gnu -emit-llvm %s -o 
- | FileCheck %s
+// expected-no-diagnostics
+
+void foo() {
+#pragma omp target
+  {}
+}
+
+// CHECK-DAG: [[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]], i32, i32 }
+// CHECK-DAG: [[DEVTY:%.+]] = type { i8*, i8*, [[ENTTY]]*, [[ENTTY]]* }
+// CHECK-DAG: [[DSCTY:%.+]] = type { i32, [[DEVTY]]*, [[ENTTY]]*, [[ENTTY]]* }
+
+// Comdat key for the offload registration code. Should have sorted offload
+// target triples encoded into the name.
+// CHECK-DAG: 
$[[REGFN:\.omp_offloading\..+\.powerpc64le-ibm-linux-gnu\.x86_64-pc-linux-gnu+]]
 = comdat any
+
+// Check if offloading descriptor is created.
+// CHECK: [[ENTBEGIN:@.+]] = external constant [[ENTTY]]
+// CHECK: [[ENTEND:@.+]] = external constant [[ENTTY]]
+// CHECK: [[DEV1BEGIN:@.+]] = extern_weak constant i8
+// CHECK: [[DEV1END:@.+]] = extern_weak constant i8
+// CHECK: [[DEV2BEGIN:@.+]] = extern_weak constant i8
+// CHECK: [[DEV2END:@.+]] = extern_weak constant i8
+// CHECK: [[IMAGES:@.+]] = internal unnamed_addr constant [2 x [[DEVTY]]] 
[{{.+}} { i8* [[DEV1BEGIN]], i8* [[DEV1END]], [[ENTTY]]* [[ENTBEGIN]], 
[[ENTTY]]* [[ENTEND]] }, {{.+}} { i8* [[DEV2BEGIN]], i8* [[DEV2END]], 
[[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }], comdat($[[REGFN]])
+// CHECK: [[DESC:@.+]] = internal constant [[DSCTY]] { i32 2, [[DEVTY]]* 
getelementptr inbounds ([2 x [[DEVTY]]], [2 x [[DEVTY]]]* [[IMAGES]], i32 0, 
i32 0), [[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }, comdat($[[REGFN]])
+
+// Check target registration is registered as a Ctor.
+// CHECK: appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* 
} { i32 0, void ()* @[[REGFN]], i8* bitcast (void ()* @[[REGFN]] to i8*) }]
+
+// Check presence of foo() and the outlined target region
+// CHECK: define void [[FOO:@.+]]()
+// CHECK: define internal void [[OUTLINEDTARGET:@.+]]() 
+
+// Check registration and unregistration code.
+
+// CHECK: define internal void @[[UNREGFN:.+]](i8*)
+// CHECK-SAME: comdat($[[REGFN]]) {
+// CHECK: call i32 @__tgt_unregister_lib([[DSCTY]]* [[DESC]])
+// CHECK: ret void
+// CHECK: declare i32 @__tgt_unregister_lib([[DSCTY]]*)
+
+// CHECK: define linkonce hidden void @[[REGFN]]()
+// CHECK-SAME: comdat {
+// CHECK: call i32 @__tgt_register_lib([[DSCTY]]* [[DESC]])
+// CHECK: call i32 @__cxa_atexit(void (i8*)* @[[UNREGFN]], i8* bitcast 
([[DSCTY]]* [[DESC]] to i8*),
+// CHECK: ret void
+// CHECK: declare i32 @__tgt_register_lib([[DSCTY]]*)
+


Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,17 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat key for the registration/unregistration code for
+// this particular combination of offloading targets.
+SmallVe

[PATCH] D50218: [OpenMP] Encode offload target triples into comdat key for offload initialization code

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 159074.

https://reviews.llvm.org/D50218

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  test/OpenMP/openmp_offload_registration.cpp


Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,19 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat key for the registration/unregistration code for
+// this particular combination of offloading targets.
+SmallVector RegFnNameParts(Devices.size() + 2U);
+RegFnNameParts[0] = "omp_offloading";
+RegFnNameParts[1] = "descriptor_reg";
+llvm::transform(Devices, std::next(RegFnNameParts.begin(), 2),
+[](const llvm::Triple &T) -> const std::string& {
+  return T.getTriple();
+});
+llvm::sort(std::next(RegFnNameParts.begin(), 2), RegFnNameParts.end());
+std::string Descriptor = getName(RegFnNameParts);
 RegFn = CGM.CreateGlobalInitOrDestructFunction(FTy, Descriptor, FI);
 CGF.StartFunction(GlobalDecl(), C.VoidTy, RegFn, FI, FunctionArgList());
 CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__tgt_register_lib), Desc);
Index: test/OpenMP/openmp_offload_registration.cpp
===
--- test/OpenMP/openmp_offload_registration.cpp
+++ test/OpenMP/openmp_offload_registration.cpp
@@ -0,0 +1,49 @@
+// Test for offload registration code for two targets
+// RUN: %clang_cc1 -verify -fopenmp -x c -triple x86_64-unknown-linux-gnu 
-fopenmp-targets=x86_64-pc-linux-gnu,powerpc64le-ibm-linux-gnu -emit-llvm %s -o 
- | FileCheck %s
+// expected-no-diagnostics
+
+void foo() {
+#pragma omp target
+  {}
+}
+
+// CHECK-DAG: [[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]], i32, i32 }
+// CHECK-DAG: [[DEVTY:%.+]] = type { i8*, i8*, [[ENTTY]]*, [[ENTTY]]* }
+// CHECK-DAG: [[DSCTY:%.+]] = type { i32, [[DEVTY]]*, [[ENTTY]]*, [[ENTTY]]* }
+
+// Comdat key for the offload registration code. Should have sorted offload
+// target triples encoded into the name.
+// CHECK-DAG: 
$[[REGFN:\.omp_offloading\..+\.powerpc64le-ibm-linux-gnu\.x86_64-pc-linux-gnu+]]
 = comdat any
+
+// Check if offloading descriptor is created.
+// CHECK: [[ENTBEGIN:@.+]] = external constant [[ENTTY]]
+// CHECK: [[ENTEND:@.+]] = external constant [[ENTTY]]
+// CHECK: [[DEV1BEGIN:@.+]] = extern_weak constant i8
+// CHECK: [[DEV1END:@.+]] = extern_weak constant i8
+// CHECK: [[DEV2BEGIN:@.+]] = extern_weak constant i8
+// CHECK: [[DEV2END:@.+]] = extern_weak constant i8
+// CHECK: [[IMAGES:@.+]] = internal unnamed_addr constant [2 x [[DEVTY]]] 
[{{.+}} { i8* [[DEV1BEGIN]], i8* [[DEV1END]], [[ENTTY]]* [[ENTBEGIN]], 
[[ENTTY]]* [[ENTEND]] }, {{.+}} { i8* [[DEV2BEGIN]], i8* [[DEV2END]], 
[[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }], comdat($[[REGFN]])
+// CHECK: [[DESC:@.+]] = internal constant [[DSCTY]] { i32 2, [[DEVTY]]* 
getelementptr inbounds ([2 x [[DEVTY]]], [2 x [[DEVTY]]]* [[IMAGES]], i32 0, 
i32 0), [[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }, comdat($[[REGFN]])
+
+// Check target registration is registered as a Ctor.
+// CHECK: appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* 
} { i32 0, void ()* @[[REGFN]], i8* bitcast (void ()* @[[REGFN]] to i8*) }]
+
+// Check presence of foo() and the outlined target region
+// CHECK: define void [[FOO:@.+]]()
+// CHECK: define internal void [[OUTLINEDTARGET:@.+]]() 
+
+// Check registration and unregistration code.
+
+// CHECK: define internal void @[[UNREGFN:.+]](i8*)
+// CHECK-SAME: comdat($[[REGFN]]) {
+// CHECK: call i32 @__tgt_unregister_lib([[DSCTY]]* [[DESC]])
+// CHECK: ret void
+// CHECK: declare i32 @__tgt_unregister_lib([[DSCTY]]*)
+
+// CHECK: define linkonce hidden void @[[REGFN]]()
+// CHECK-SAME: comdat {
+// CHECK: call i32 @__tgt_register_lib([[DSCTY]]* [[DESC]])
+// CHECK: call i32 @__cxa_atexit(void (i8*)* @[[UNREGFN]], i8* bitcast 
([[DSCTY]]* [[DESC]] to i8*),
+// CHECK: ret void
+// CHECK: declare i32 @__tgt_register_lib([[DSCTY]]*)
+


Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,19 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat key fo

[PATCH] D50218: [OpenMP] Encode offload target triples into comdat key for offload initialization code

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC338916: [OpenMP] Encode offload target triples into comdat 
key for offload… (authored by sdmitriev, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D50218

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  test/OpenMP/openmp_offload_registration.cpp


Index: test/OpenMP/openmp_offload_registration.cpp
===
--- test/OpenMP/openmp_offload_registration.cpp
+++ test/OpenMP/openmp_offload_registration.cpp
@@ -0,0 +1,49 @@
+// Test for offload registration code for two targets
+// RUN: %clang_cc1 -verify -fopenmp -x c -triple x86_64-unknown-linux-gnu 
-fopenmp-targets=x86_64-pc-linux-gnu,powerpc64le-ibm-linux-gnu -emit-llvm %s -o 
- | FileCheck %s
+// expected-no-diagnostics
+
+void foo() {
+#pragma omp target
+  {}
+}
+
+// CHECK-DAG: [[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]], i32, i32 }
+// CHECK-DAG: [[DEVTY:%.+]] = type { i8*, i8*, [[ENTTY]]*, [[ENTTY]]* }
+// CHECK-DAG: [[DSCTY:%.+]] = type { i32, [[DEVTY]]*, [[ENTTY]]*, [[ENTTY]]* }
+
+// Comdat key for the offload registration code. Should have sorted offload
+// target triples encoded into the name.
+// CHECK-DAG: 
$[[REGFN:\.omp_offloading\..+\.powerpc64le-ibm-linux-gnu\.x86_64-pc-linux-gnu+]]
 = comdat any
+
+// Check if offloading descriptor is created.
+// CHECK: [[ENTBEGIN:@.+]] = external constant [[ENTTY]]
+// CHECK: [[ENTEND:@.+]] = external constant [[ENTTY]]
+// CHECK: [[DEV1BEGIN:@.+]] = extern_weak constant i8
+// CHECK: [[DEV1END:@.+]] = extern_weak constant i8
+// CHECK: [[DEV2BEGIN:@.+]] = extern_weak constant i8
+// CHECK: [[DEV2END:@.+]] = extern_weak constant i8
+// CHECK: [[IMAGES:@.+]] = internal unnamed_addr constant [2 x [[DEVTY]]] 
[{{.+}} { i8* [[DEV1BEGIN]], i8* [[DEV1END]], [[ENTTY]]* [[ENTBEGIN]], 
[[ENTTY]]* [[ENTEND]] }, {{.+}} { i8* [[DEV2BEGIN]], i8* [[DEV2END]], 
[[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }], comdat($[[REGFN]])
+// CHECK: [[DESC:@.+]] = internal constant [[DSCTY]] { i32 2, [[DEVTY]]* 
getelementptr inbounds ([2 x [[DEVTY]]], [2 x [[DEVTY]]]* [[IMAGES]], i32 0, 
i32 0), [[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }, comdat($[[REGFN]])
+
+// Check target registration is registered as a Ctor.
+// CHECK: appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* 
} { i32 0, void ()* @[[REGFN]], i8* bitcast (void ()* @[[REGFN]] to i8*) }]
+
+// Check presence of foo() and the outlined target region
+// CHECK: define void [[FOO:@.+]]()
+// CHECK: define internal void [[OUTLINEDTARGET:@.+]]() 
+
+// Check registration and unregistration code.
+
+// CHECK: define internal void @[[UNREGFN:.+]](i8*)
+// CHECK-SAME: comdat($[[REGFN]]) {
+// CHECK: call i32 @__tgt_unregister_lib([[DSCTY]]* [[DESC]])
+// CHECK: ret void
+// CHECK: declare i32 @__tgt_unregister_lib([[DSCTY]]*)
+
+// CHECK: define linkonce hidden void @[[REGFN]]()
+// CHECK-SAME: comdat {
+// CHECK: call i32 @__tgt_register_lib([[DSCTY]]* [[DESC]])
+// CHECK: call i32 @__cxa_atexit(void (i8*)* @[[UNREGFN]], i8* bitcast 
([[DSCTY]]* [[DESC]] to i8*),
+// CHECK: ret void
+// CHECK: declare i32 @__tgt_register_lib([[DSCTY]]*)
+
Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -3818,7 +3818,19 @@
 CGF.disableDebugInfo();
 const auto &FI = CGM.getTypes().arrangeNullaryFunction();
 llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI);
-std::string Descriptor = getName({"omp_offloading", "descriptor_reg"});
+
+// Encode offload target triples into the registration function name. It
+// will serve as a comdat key for the registration/unregistration code for
+// this particular combination of offloading targets.
+SmallVector RegFnNameParts(Devices.size() + 2U);
+RegFnNameParts[0] = "omp_offloading";
+RegFnNameParts[1] = "descriptor_reg";
+llvm::transform(Devices, std::next(RegFnNameParts.begin(), 2),
+[](const llvm::Triple &T) -> const std::string& {
+  return T.getTriple();
+});
+llvm::sort(std::next(RegFnNameParts.begin(), 2), RegFnNameParts.end());
+std::string Descriptor = getName(RegFnNameParts);
 RegFn = CGM.CreateGlobalInitOrDestructFunction(FTy, Descriptor, FI);
 CGF.StartFunction(GlobalDecl(), C.VoidTy, RegFn, FI, FunctionArgList());
 CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__tgt_register_lib), Desc);


Index: test/OpenMP/openmp_offload_registration.cpp
===
--- test/OpenMP/openmp_offload_registration.cpp
+++ test/OpenMP/openmp_offload_registration.cpp
@@ -0,0 +1,49 @@
+// Test for offload registration code for two targets
+// RUN: %clang_cc1 -verify -fopenmp -x c -triple x86_64-unknown-linux-gnu -fopenm

[PATCH] D33509: [OpenMP] Create COMDAT group for OpenMP offload registration code to avoid multiple copies

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

I do not have commit access yet, so I cannot commit this patch myself. Can you 
please check it in for me?


https://reviews.llvm.org/D33509



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

[PATCH] D56811: [Mem2Reg] Enable promotion for bitcastable load/store values

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev created this revision.
sdmitriev added a reviewer: andrew.w.kaylor.
Herald added subscribers: cfe-commits, javed.absar.

This patch enables Mem2Reg to handle loads/stores from/to bitcasted alloca
values as long as the loaded/stored value is bitcastable to the allocated
type (see example below). Such instruction sequences can be introduced by
the InstCombine pass as part of the load canonicalization.


  %f = alloca float, align 4
  ...
  %0 = getelementptr inbounds float, float* %A, i64 %idx
  %1 = bitcast float* %0 to i32*
  %2 = load i32, i32* %1, align 4
  %3 = bitcast float* %f to i32*
  store i32 %2, i32* %3, align 4


Repository:
  rC Clang

https://reviews.llvm.org/D56811

Files:
  test/CodeGen/aarch64-neon-vget.c
  test/CodeGen/arm_neon_intrinsics.c

Index: test/CodeGen/aarch64-neon-vget.c
===
--- test/CodeGen/aarch64-neon-vget.c
+++ test/CodeGen/aarch64-neon-vget.c
@@ -80,18 +80,12 @@
 }
 
 // CHECK-LABEL: define float @test_vget_lane_f16(<4 x half> %a) #0 {
-// CHECK:   [[__REINT_242:%.*]] = alloca <4 x half>, align 8
-// CHECK:   [[__REINT1_242:%.*]] = alloca i16, align 2
-// CHECK:   store <4 x half> %a, <4 x half>* [[__REINT_242]], align 8
-// CHECK:   [[TMP0:%.*]] = bitcast <4 x half>* [[__REINT_242]] to <4 x i16>*
-// CHECK:   [[TMP1:%.*]] = load <4 x i16>, <4 x i16>* [[TMP0]], align 8
-// CHECK:   [[TMP2:%.*]] = bitcast <4 x i16> [[TMP1]] to <8 x i8>
-// CHECK:   [[TMP3:%.*]] = bitcast <8 x i8> [[TMP2]] to <4 x i16>
-// CHECK:   [[VGET_LANE:%.*]] = extractelement <4 x i16> [[TMP3]], i32 1
-// CHECK:   store i16 [[VGET_LANE]], i16* [[__REINT1_242]], align 2
-// CHECK:   [[TMP4:%.*]] = bitcast i16* [[__REINT1_242]] to half*
-// CHECK:   [[TMP5:%.*]] = load half, half* [[TMP4]], align 2
-// CHECK:   [[CONV:%.*]] = fpext half [[TMP5]] to float
+// CHECK:   [[TMP0:%.*]] = bitcast <4 x half> %a to <4 x i16>
+// CHECK:   [[TMP1:%.*]] = bitcast <4 x i16> [[TMP0]] to <8 x i8>
+// CHECK:   [[TMP2:%.*]] = bitcast <8 x i8> [[TMP1]] to <4 x i16>
+// CHECK:   [[VGET_LANE:%.*]] = extractelement <4 x i16> [[TMP2]], i32 1
+// CHECK:   [[TMP3:%.*]] = bitcast i16 [[VGET_LANE]] to half
+// CHECK:   [[CONV:%.*]] = fpext half [[TMP3]] to float
 // CHECK:   ret float [[CONV]]
 float32_t test_vget_lane_f16(float16x4_t a) {
   return vget_lane_f16(a, 1);
@@ -173,18 +167,12 @@
 }
 
 // CHECK-LABEL: define float @test_vgetq_lane_f16(<8 x half> %a) #1 {
-// CHECK:   [[__REINT_244:%.*]] = alloca <8 x half>, align 16
-// CHECK:   [[__REINT1_244:%.*]] = alloca i16, align 2
-// CHECK:   store <8 x half> %a, <8 x half>* [[__REINT_244]], align 16
-// CHECK:   [[TMP0:%.*]] = bitcast <8 x half>* [[__REINT_244]] to <8 x i16>*
-// CHECK:   [[TMP1:%.*]] = load <8 x i16>, <8 x i16>* [[TMP0]], align 16
-// CHECK:   [[TMP2:%.*]] = bitcast <8 x i16> [[TMP1]] to <16 x i8>
-// CHECK:   [[TMP3:%.*]] = bitcast <16 x i8> [[TMP2]] to <8 x i16>
-// CHECK:   [[VGETQ_LANE:%.*]] = extractelement <8 x i16> [[TMP3]], i32 3
-// CHECK:   store i16 [[VGETQ_LANE]], i16* [[__REINT1_244]], align 2
-// CHECK:   [[TMP4:%.*]] = bitcast i16* [[__REINT1_244]] to half*
-// CHECK:   [[TMP5:%.*]] = load half, half* [[TMP4]], align 2
-// CHECK:   [[CONV:%.*]] = fpext half [[TMP5]] to float
+// CHECK:   [[TMP0:%.*]] = bitcast <8 x half> %a to <8 x i16>
+// CHECK:   [[TMP1:%.*]] = bitcast <8 x i16> [[TMP0]] to <16 x i8>
+// CHECK:   [[TMP2:%.*]] = bitcast <16 x i8> [[TMP1]] to <8 x i16>
+// CHECK:   [[VGETQ_LANE:%.*]] = extractelement <8 x i16> [[TMP2]], i32 3
+// CHECK:   [[TMP3:%.*]] = bitcast i16 [[VGETQ_LANE]] to half
+// CHECK:   [[CONV:%.*]] = fpext half [[TMP3]] to float
 // CHECK:   ret float [[CONV]]
 float32_t test_vgetq_lane_f16(float16x8_t a) {
   return vgetq_lane_f16(a, 3);
@@ -303,23 +291,14 @@
 }
 
 // CHECK-LABEL: define <4 x half> @test_vset_lane_f16(half* %a, <4 x half> %b) #0 {
-// CHECK:   [[__REINT_246:%.*]] = alloca half, align 2
-// CHECK:   [[__REINT1_246:%.*]] = alloca <4 x half>, align 8
-// CHECK:   [[__REINT2_246:%.*]] = alloca <4 x i16>, align 8
 // CHECK:   [[TMP0:%.*]] = load half, half* %a, align 2
-// CHECK:   store half [[TMP0]], half* [[__REINT_246]], align 2
-// CHECK:   store <4 x half> %b, <4 x half>* [[__REINT1_246]], align 8
-// CHECK:   [[TMP1:%.*]] = bitcast half* [[__REINT_246]] to i16*
-// CHECK:   [[TMP2:%.*]] = load i16, i16* [[TMP1]], align 2
-// CHECK:   [[TMP3:%.*]] = bitcast <4 x half>* [[__REINT1_246]] to <4 x i16>*
-// CHECK:   [[TMP4:%.*]] = load <4 x i16>, <4 x i16>* [[TMP3]], align 8
-// CHECK:   [[TMP5:%.*]] = bitcast <4 x i16> [[TMP4]] to <8 x i8>
-// CHECK:   [[TMP6:%.*]] = bitcast <8 x i8> [[TMP5]] to <4 x i16>
-// CHECK:   [[VSET_LANE:%.*]] = insertelement <4 x i16> [[TMP6]], i16 [[TMP2]], i32 3
-// CHECK:   store <4 x i16> [[VSET_LANE]], <4 x i16>* [[__REINT2_246]], align 8
-// CHECK:   [[TMP7:%.*]] = bitcast <4 x i16>* [[__REINT2_246]] to <4 x half>*
-// CHECK:   [[TMP8:%.*]] = load <4 x half>, <4 x half>* [[TMP7]], align 

[PATCH] D56811: [Mem2Reg] Enable promotion for bitcastable load/store values

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

This patch is a clang's part of this review https://reviews.llvm.org/D56810


Repository:
  rC Clang

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

https://reviews.llvm.org/D56811



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://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 created this revision.
sdmitriev added reviewers: hfinkel, ABataev.
Herald added subscribers: cfe-commits, guansong, mgorny.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

This is the first set of changes for eliminating OpenMP linker script that adds 
new clang-offload-wrapper tool (please see https://reviews.llvm.org/D64943 for 
more details). This tool takes device images as input and produces a wrapper 
bit-code file which contains device images as data and offload registration 
code that registers the binaries in OpenMP offloading runtime at startup.


Repository:
  rC Clang

https://reviews.llvm.org/D65130

Files:
  clang/test/Driver/clang-offload-wrapper.ll
  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,290 @@
+//===-- 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/Error.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
+#include 
+#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;
+
+  using MemoryBuffersVector = SmallVectorImpl>;
+
+private:
+  IntegerType *getSizeTTy() {
+auto PtrSize = M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C));
+return PtrSize == 8 ? Type::getInt64Ty(C) : Type::getInt32Ty(C);
+  }
+
+  // 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_o

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

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Any feedback?


Repository:
  rC Clang

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] D65130: [clang][OpenMP] Add clang-offload-wrapper tool

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 213481.
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.ll
  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,290 @@
+//===-- 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/Error.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
+#include 
+#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;
+
+  using MemoryBuffersVector = SmallVectorImpl>;
+
+private:
+  IntegerType *getSizeTTy() {
+auto PtrSize = M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C));
+return PtrSize == 8 ? Type::getInt64Ty(C) : Type::getInt32Ty(C);
+  }
+
+  // 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(),
+  getEntryPtrTy());
+return DescTy;
+  }
+
+  PointerType *getBinDescPtrTy() {
+return PointerType::getUnqual(getBinDescTy());
+  }
+
+  Glob

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

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked 4 inline comments as done.
sdmitriev added inline comments.



Comment at: clang/include/clang/Driver/Action.h:74
 OffloadUnbundlingJobClass,
+OffloadWrapperJobClass,
 

ABataev wrote:
> Do we really need this new kind of job here, can we use bundler instead?
Well, I can probably try to reuse bundling action for wrapping, but I think it 
will just complicate the logic. Wrapping logically differs from bundling and 
wrapping is done by a different tool, so I think it is natural to add a 
distinct action class for it.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9748
+  // If we have offloading in the current module, we need to emit the entries.
   createOffloadEntriesAndInfoMetadata();
 }

ABataev wrote:
> Do not emit it for the devices and simd only mode. Also, would be good to 
> assert if no devices triples were specified.
Offload entries are actually emitted both for host and target compilations. I 
have added a check for OpenMP simd mode to 
createOffloadingBinaryDescriptorRegistration().



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:1470
+  /// was emitted in the current module.
+  virtual void emitOffloadTables();
 

ABataev wrote:
> Ithink, you can drop `virtual` here and remove overridden version from the 
> CGOpenMPRuntimeSimd. Instead, just check for OpenMP simd mode in the original 
> function and just early exit in this case.
Ok. Without virtual I do not see much reasons for adding new function which 
just calls createOffloadEntriesAndInfoMetadata(), so instead I have just made 
createOffloadEntriesAndInfoMetadata() public and added a check for OpenMP simd 
mode to this function.


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 marked 14 inline comments as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:69
+
+  using MemoryBuffersVector = SmallVectorImpl>;
+

ABataev wrote:
> Why not `ArrayRef`?
Changed to MemoryBufferRef which in some sense is similar to ArrayRef.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:72-75
+  IntegerType *getSizeTTy() {
+auto PtrSize = M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C));
+return PtrSize == 8 ? Type::getInt64Ty(C) : Type::getInt32Ty(C);
+  }

ABataev wrote:
> Not sure that this is the best solution, we may end up with incorrect 
> `size_t` type in some cases. 
 I have slightly revised this code to avoid unexpected results.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:148
+
+  auto *Image = new GlobalVariable(M, Data->getType(), true,
+  GlobalVariable::InternalLinkage,

ABataev wrote:
> Seems to me the code is not formatted
Right. Fixed.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:209
+  void createUnregisterFunction(GlobalVariable *BinDesc) {
+auto *FuncTy = FunctionType::get(Type::getVoidTy(C), {}, false);
+auto *Func = Function::Create(FuncTy, GlobalValue::InternalLinkage,

ABataev wrote:
> `llvm::None` instead of `{}`
Switched to a different constructor which does not have argument types.


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 marked 3 inline comments as done.
sdmitriev added inline comments.



Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:226
+// Add this function to global destructors.
+appendToGlobalDtors(M, Func, 0);
+  }

vzakhari wrote:
> FYI, llvm.global_dtors does not work on Windows.  The symbol will be placed 
> into .CRT$XC[A-Z] portion of .CRT in TargetLoweringObjectFileImpl.cpp, so, 
> basically, __tgt_unregister_lib will be called right after 
> __tgt_register_lib.  We can either use a trick from ASAN, i.e. put 
> llvm.global_dtors into .CRT$XT[A-Z] (I am not sure how solid this solution 
> is) or call atexit() inside __tgt_register_lib to register 
> __tgt_unregister_lib terminator.
It works as expected on Linux, so I guess this is just a bug in lowering code 
for Windows that need to be fixed.


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.

As I understand ‘atexit’ solution would be target dependent (‘__cxa_atexit’ on 
Linux and ‘atexit’ on Windows) whereas @llvm.global_ctors/dtors variables offer 
similar and platform neutral functionality 
(http://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable). Why 
do you think that ‘atexit’ is a better choice?


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 marked 2 inline comments as done.
sdmitriev added inline comments.



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

ABataev wrote:
> 1. Markt it `const`.
> 2. This still is not the best solution, since `size_t` not necessarily has 
> the pointer size. I don't know if there is a better solution. @hfinkel? If 
> this is the best, why not just to use `getIntPtrType(C)`?
It cannot be const because of Type::getIntXXTy(LLVMContext &C) calls.



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

ABataev wrote:
> Use `ArrayRef` instead of `const SmallVectorImpl &`
Done. And I have also changed MemoryBufferRef => ArrayRef (as you earlier 
suggested).


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 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 &Input) = 0;
+  virtual Error ReadHeader(MemoryBuffer &Input) = 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 &Input) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer &Input) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer &Input) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer &Input) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 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 &OS,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream &OS,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream &OS,
+ 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 &OS, StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -222,7 +226,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer &Input) final {
+  Error ReadHeader(MemoryBuffer &Input) 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)

[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(),
+  getEntryPtrTy(

[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] 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#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#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#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] 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] 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] 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] 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] 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] 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] 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 &AL) {}
 
-/// 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] 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 &AL) {}
 
-/// 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 &DA) {}
 
 /// 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 &AL) {}
 
-/// 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 &DA) {}
 
 /// 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]

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&id=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 &AL) {}
 
-/// 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 &DA) {}
 
 /// 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 &AL) {}
 
-/// 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 &DA) {}
 
 /// 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] 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 &Input) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer &Input) = 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] 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 &Bin : 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,
+ GlobalVar

[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 &Bin : 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 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 &Bin : 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]

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&id=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 &Bin : 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

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

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] 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 &Buf : 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] 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 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 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] 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 &Input) = 0;
+  /// file.
+  virtual Error ReadHeader(MemoryBuffer &Input) = 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 &Input) = 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 &Input) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer &Input) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer &Input) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 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 &OS,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream &OS,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream &OS,
+ 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 &OS, StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -224,7 +228,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer &Input) final {
+  Error ReadHeader(MemoryBuffer &Input) 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);
   ReadCh

[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 &Input) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer &Input) = 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] 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] 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] 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] 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] 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 &Input) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer &Input) = 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] 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&id=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 &Input) = 0;
+  /// file.
+  virtual Error ReadHeader(MemoryBuffer &Input) = 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 &Input) = 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 &Input) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer &Input) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer &Input) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 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 &OS,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream &OS,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream &OS,
+ 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 &OS, StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -224,7 +228,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer &Input) final {
+  Error ReadHeader(MemoryBuffer &Input) 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);
  

[PATCH] D66485: [Clang][Bundler] Use llvm-objcopy for creating fat object files

[Sergey Dmitriev via Phabricator via cfe-commits]

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

clang-offload-bundler currently uses partial linking for creating fat object 
files, but such technique cannot be used on Windows due to the absence of 
partial linking support in the linker. This patch changes implementation to use 
llvm-objcopy for merging device and host objects instead of doing partial 
linking. This is one step forward towards enabling OpenMP offload on Windows.


Repository:
  rC Clang

https://reviews.llvm.org/D66485

Files:
  clang/test/CMakeLists.txt
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/CMakeLists.txt
  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
@@ -21,12 +21,6 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
-#include "llvm/Bitcode/BitcodeWriter.h"
-#include "llvm/IR/Constant.h"
-#include "llvm/IR/Constants.h"
-#include "llvm/IR/GlobalVariable.h"
-#include "llvm/IR/LLVMContext.h"
-#include "llvm/IR/Module.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
@@ -39,6 +33,7 @@
 #include "llvm/Support/Program.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 #include 
@@ -94,11 +89,6 @@
  "instead of actually executing them - for testing purposes.\n"),
 cl::init(false), cl::cat(ClangOffloadBundlerCategory));
 
-static cl::opt DumpTemporaryFiles(
-"dump-temporary-files",
-cl::desc("Dumps any temporary files created - for testing purposes.\n"),
-cl::init(false), cl::cat(ClangOffloadBundlerCategory));
-
 /// Magic string that marks the existence of offloading data.
 #define OFFLOAD_BUNDLER_MAGIC_STR "__CLANG_OFFLOAD_BUNDLE__"
 
@@ -116,12 +106,6 @@
   OffloadKind = KindTriplePair.first;
   Triple = KindTriplePair.second;
 }
-static StringRef getTriple(StringRef Target) {
-  StringRef OffloadKind;
-  StringRef Triple;
-  getOffloadKindAndTriple(Target, OffloadKind, Triple);
-  return Triple;
-}
 static bool hasHostKind(StringRef Target) {
   StringRef OffloadKind;
   StringRef Triple;
@@ -410,19 +394,6 @@
   /// read from the buffers.
   unsigned NumberOfProcessedInputs = 0;
 
-  /// LLVM context used to create the auxiliary modules.
-  LLVMContext VMContext;
-
-  /// LLVM module used to create an object with all the bundle
-  /// components.
-  std::unique_ptr AuxModule;
-
-  /// The current triple we are working with.
-  StringRef CurrentTriple;
-
-  /// The name of the main input file.
-  StringRef MainInputFileName;
-
   /// Iterator of the current and next section.
   section_iterator CurrentSection;
   section_iterator NextSection;
@@ -476,19 +447,10 @@
 
 // Record number of inputs.
 NumberOfInputs = Inputs.size();
-
-// Create an LLVM module to have the content we need to bundle.
-auto *M = new Module("clang-offload-bundle", VMContext);
-M->setTargetTriple(getTriple(TargetNames[HostInputIndex]));
-AuxModule.reset(M);
   }
 
   void WriteBundleStart(raw_fd_ostream &OS, StringRef TargetTriple) final {
 ++NumberOfProcessedInputs;
-
-// Record the triple we are using, that will be used to name the section we
-// will create.
-CurrentTriple = TargetTriple;
   }
 
   bool WriteBundleEnd(raw_fd_ostream &OS, StringRef TargetTriple) final {
@@ -500,76 +462,39 @@
 if (NumberOfProcessedInputs != NumberOfInputs)
   return false;
 
-// Create the bitcode file name to write the resulting code to. Keep it if
-// save-temps is active.
-SmallString<128> BitcodeFileName;
-if (sys::fs::createTemporaryFile("clang-offload-bundler", "bc",
- BitcodeFileName)) {
-  errs() << "error: unable to create temporary file.\n";
+// Find llvm-objcopy in order to create the bundle binary.
+ErrorOr Objcopy = sys::findProgramByName(
+"llvm-objcopy", sys::path::parent_path(BundlerExecutable));
+if (!Objcopy) {
+  errs() << "error: unable to find 'llvm-objcopy' in path.\n";
   return true;
 }
 
-// Dump the contents of the temporary file if that was requested.
-if (DumpTemporaryFiles) {
-  errs() << ";\n; Object file bundler IR file.\n;\n";
-  AuxModule.get()->print(errs(), nullptr,
- /*ShouldPreserveUseListOrder=*/false,
- /*IsForDebug=*/true);
-  errs() << '\n';
-}
-
-// Find clang in order to create the bundle binary.
-StringRef Dir = sys

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

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D64943#1633164 , @ABataev wrote:

> In D64943#1619958 , @sdmitriev wrote:
>
> > As I understand ‘atexit’ solution would be target dependent (‘__cxa_atexit’ 
> > on Linux and ‘atexit’ on Windows) whereas @llvm.global_ctors/dtors 
> > variables offer similar and platform neutral functionality 
> > (http://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable). 
> > Why do you think that ‘atexit’ is a better choice?
>
>
> Because it does not work on Windows, better to have portable solution, if 
> possible.


@vzakhari has already committed his fix for llvm.global_dtors 
(https://reviews.llvm.org/D66373), so I assume use of llvm.global_dtors in this 
patch would no longer cause problems 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] D66598: [Clang][Bundler] Fix for a hang when unbundling fat binary

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

clang-offload-bundler tool may hang under certain conditions when it extracts a 
subset of all available device bundles from the fat binary that is handled by 
the BinaryFileHandler. This patch fixes this problem.


Repository:
  rC Clang

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
@@ -231,6 +231,7 @@
 
   /// Iterator for the bundle information that is being read.
   StringMap::iterator CurBundleInfo;
+  StringMap::iterator NextBundleInfo;
 
 public:
   BinaryFileHandler() : FileHandler() {}
@@ -300,13 +301,14 @@
   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 &Input) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
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
@@ -231,6 +231,7 @@
 
   /// Iterator for the bundle information that is being read.
   StringMap::iterator CurBundleInfo;
+  StringMap::iterator NextBundleInfo;
 
 public:
   BinaryFileHandler() : FileHandler() {}
@@ -300,13 +301,14 @@
   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 &Input) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
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] D66601: [Clang][Bundler] Do not require host triple for extracting device bundles

[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.
sdmitriev added a reviewer: hfinkel.

Bundler currently requires host triple to be provided no matter if you are 
performing bundling or unbundling, but for unbundling operation such 
requirement is too restrictive. You may for example want to examine device part 
of the object for a particular offload target, but you have to extract host 
part as well even though you do not need it. Host triple isn't really needed 
for unbundling, so this patch removes that requirement.


Repository:
  rC Clang

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
@@ -833,7 +833,6 @@
 
   // Read all the bundles that are in the work list. If we find no bundles we
   // assume the file is meant for the host target.
-  bool FoundHostBundle = false;
   while (!Worklist.empty()) {
 StringRef CurTriple = FH.get()->ReadBundleStart(Input);
 
@@ -859,10 +858,6 @@
 FH.get()->ReadBundle(OutputFile, Input);
 FH.get()->ReadBundleEnd(Input);
 Worklist.erase(Output);
-
-// Record if we found the host bundle.
-if (hasHostKind(CurTriple))
-  FoundHostBundle = true;
   }
 
   // If no bundles were found, assume the input file is the host bundle and
@@ -884,12 +879,6 @@
 return false;
   }
 
-  // If we found elements, we emit an error if none of those were for the host.
-  if (!FoundHostBundle) {
-errs() << "error: Can't find bundle for the host target\n";
-return true;
-  }
-
   // If we still have any elements in the worklist, create empty files for 
them.
   for (auto &E : Worklist) {
 std::error_code EC;
@@ -988,7 +977,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
@@ -183,6 +183,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=s 
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 
-inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
 //
 // Check binary bundle/unbundle. The content that we have before bundling must 
be the same we have after unbundling.
 //
@@ -221,6 +225,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// 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
+
 //
 // 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
@@ -256,6 +264,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=o 
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 
-inputs=%t.bundle3.o -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
 // Some code so that we can create a binary out of this file.
 int A = 0;
 void test_func(void) {


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -833,7 +833,6 @@
 
   // Read all the bundles that are in the work list. If we find no bundles we
   // assume the file is meant for the host target.
-  bool FoundHostBundle = false;
   while (!Worklist.empty()) {
 StringRef CurTriple = FH.get()->ReadBundleStart(Input);
 
@@ -859,10 +858,6 @@
 FH.get()->ReadBundle(OutputFile, Input);
 FH.get()->ReadBundleEnd(Input);
 Worklist.erase(Output);
-
-// Record if we found the host bundle.
-if (hasHostKind(CurTriple))
-  FoundHostBundle = true;
   }
 
   // If no bundles were found, assume the input file is the host bundle and
@@ -884,12 +879,6 @@
 return

[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] D66485: [Clang][Bundler] Use llvm-objcopy for creating fat object files

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Any feedback?


Repository:
  rC Clang

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

https://reviews.llvm.org/D66485



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

[PATCH] D66485: [Clang][Bundler] Use llvm-objcopy for creating fat object files

[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:485
+"=" + InputFileNames[I]));
+ObjcopyArgs.push_back(InputFileNames[HostInputIndex]);
+ObjcopyArgs.push_back(OutputFileNames.front());

ABataev wrote:
> As I understand, the resulting object file has the same structure as before? 
> And we still can get an access to the host object file using standard 
> programs, like readelf, objdump, etc.?
Yes, the structure is the same. The only difference is in the section flags 
that contain device objects - such sections used to have ALLOC attribute, but 
they do not have it now which I believe is a good think.

That is what we had earlier:

```
bash-4.2$ cat y.c
#pragma omp declare target
void foo() {}
#pragma omp end declare target
bash-4.2$ clang -fopenmp -fopenmp-targets=x86_64-pc-linux-gnu -c y.c
bash-4.2$ objdump -h y.o 

y.o: file format elf64-x86-64

Sections:
Idx Name  Size  VMA   LMA   File off  Algn
  0 .text 0008      0040  2**4
  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .text.startup 0010      0050  2**4
  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
  2 __CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu 03f8  
    0060  2**4
  CONTENTS, ALLOC, LOAD, READONLY, DATA
  3 __CLANG_OFFLOAD_BUNDLE__host-x86_64-unknown-linux-gnu 0630  
    0460  2**4
  CONTENTS, ALLOC, LOAD, READONLY, DATA
  4 .eh_frame 0058      0a90  2**3
  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
  5 .init_array.0 0008      0ae8  2**3
  CONTENTS, ALLOC, LOAD, RELOC, DATA
  6 .comment  006a      0af0  2**0
  CONTENTS, READONLY
  7 .note.GNU-stack       0b5a  2**0
  CONTENTS, READONLY
  8 .llvm_addrsig 0002      0b5a  2**0
  CONTENTS, READONLY, EXCLUDE
bash-4.2$ 
```

And here is what we have with this patch:

```
bash-4.2$ clang -fopenmp -fopenmp-targets=x86_64-pc-linux-gnu -c y.c
bash-4.2$ objdump -h y.o 

y.o: file format elf64-x86-64

Sections:
Idx Name  Size  VMA   LMA   File off  Algn
  0 .text 0006      0040  2**4
  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .text.startup 0010      0050  2**4
  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
  2 .init_array.0 0008      0060  2**3
  CONTENTS, ALLOC, LOAD, RELOC, DATA
  3 .comment  006a      0068  2**0
  CONTENTS, READONLY
  4 .note.GNU-stack       00d2  2**0
  CONTENTS, READONLY
  5 .eh_frame 0058      00d8  2**3
  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
  6 .llvm_addrsig 0002      0238  2**0
  CONTENTS, READONLY, EXCLUDE
  7 __CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu 03f8  
    0353  2**0
  CONTENTS, READONLY
  8 __CLANG_OFFLOAD_BUNDLE__host-x86_64-unknown-linux-gnu 0630  
    074b  2**0
  CONTENTS, READONLY
bash-4.2$ 
```



Repository:
  rC Clang

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

https://reviews.llvm.org/D66485



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

[PATCH] D66485: [Clang][Bundler] Use llvm-objcopy for creating fat object files

[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:233
+// 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]]=[[INOBJ1]]" 
"--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-powerpc64le-ibm-linux-gnu=[[INOBJ2]]"
 "--add-section=__CLANG_OFFLOAD_BUNDLE__openmp-x86_64-pc-linux-gnu=[[INOBJ3]]" 
"[[INOBJ1]]" "[[OUTOBJ]]"

ABataev wrote:
> Why does FileCheck have all these strange options?
Well, -DVAR=VALUE is a standard FileCheck option which allows to define 
variable that can be used in the check pattern (please see 
http://www.llvm.org/docs/CommandGuide/FileCheck.html)

```
-D
Sets a filecheck pattern variable VAR with value VALUE that can be used in 
CHECK: lines.
```

I am using these options for defining variables which contain input and output 
object names as well as the host triple.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66485



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

[PATCH] D66485: [Clang][Bundler] Use llvm-objcopy for creating fat object files

[Sergey Dmitriev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGed153ef044fd: [Clang][Bundler] Use llvm-objcopy for creating 
fat object files (authored by sdmitriev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66485

Files:
  clang/test/CMakeLists.txt
  clang/test/Driver/clang-offload-bundler.c
  clang/tools/clang-offload-bundler/CMakeLists.txt
  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
@@ -21,12 +21,6 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
-#include "llvm/Bitcode/BitcodeWriter.h"
-#include "llvm/IR/Constant.h"
-#include "llvm/IR/Constants.h"
-#include "llvm/IR/GlobalVariable.h"
-#include "llvm/IR/LLVMContext.h"
-#include "llvm/IR/Module.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
@@ -39,6 +33,7 @@
 #include "llvm/Support/Program.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/Signals.h"
+#include "llvm/Support/StringSaver.h"
 #include 
 #include 
 #include 
@@ -94,11 +89,6 @@
  "instead of actually executing them - for testing purposes.\n"),
 cl::init(false), cl::cat(ClangOffloadBundlerCategory));
 
-static cl::opt DumpTemporaryFiles(
-"dump-temporary-files",
-cl::desc("Dumps any temporary files created - for testing purposes.\n"),
-cl::init(false), cl::cat(ClangOffloadBundlerCategory));
-
 /// Magic string that marks the existence of offloading data.
 #define OFFLOAD_BUNDLER_MAGIC_STR "__CLANG_OFFLOAD_BUNDLE__"
 
@@ -116,12 +106,6 @@
   OffloadKind = KindTriplePair.first;
   Triple = KindTriplePair.second;
 }
-static StringRef getTriple(StringRef Target) {
-  StringRef OffloadKind;
-  StringRef Triple;
-  getOffloadKindAndTriple(Target, OffloadKind, Triple);
-  return Triple;
-}
 static bool hasHostKind(StringRef Target) {
   StringRef OffloadKind;
   StringRef Triple;
@@ -410,19 +394,6 @@
   /// read from the buffers.
   unsigned NumberOfProcessedInputs = 0;
 
-  /// LLVM context used to create the auxiliary modules.
-  LLVMContext VMContext;
-
-  /// LLVM module used to create an object with all the bundle
-  /// components.
-  std::unique_ptr AuxModule;
-
-  /// The current triple we are working with.
-  StringRef CurrentTriple;
-
-  /// The name of the main input file.
-  StringRef MainInputFileName;
-
   /// Iterator of the current and next section.
   section_iterator CurrentSection;
   section_iterator NextSection;
@@ -476,19 +447,10 @@
 
 // Record number of inputs.
 NumberOfInputs = Inputs.size();
-
-// Create an LLVM module to have the content we need to bundle.
-auto *M = new Module("clang-offload-bundle", VMContext);
-M->setTargetTriple(getTriple(TargetNames[HostInputIndex]));
-AuxModule.reset(M);
   }
 
   void WriteBundleStart(raw_fd_ostream &OS, StringRef TargetTriple) final {
 ++NumberOfProcessedInputs;
-
-// Record the triple we are using, that will be used to name the section we
-// will create.
-CurrentTriple = TargetTriple;
   }
 
   bool WriteBundleEnd(raw_fd_ostream &OS, StringRef TargetTriple) final {
@@ -500,76 +462,39 @@
 if (NumberOfProcessedInputs != NumberOfInputs)
   return false;
 
-// Create the bitcode file name to write the resulting code to. Keep it if
-// save-temps is active.
-SmallString<128> BitcodeFileName;
-if (sys::fs::createTemporaryFile("clang-offload-bundler", "bc",
- BitcodeFileName)) {
-  errs() << "error: unable to create temporary file.\n";
+// Find llvm-objcopy in order to create the bundle binary.
+ErrorOr Objcopy = sys::findProgramByName(
+"llvm-objcopy", sys::path::parent_path(BundlerExecutable));
+if (!Objcopy) {
+  errs() << "error: unable to find 'llvm-objcopy' in path.\n";
   return true;
 }
 
-// Dump the contents of the temporary file if that was requested.
-if (DumpTemporaryFiles) {
-  errs() << ";\n; Object file bundler IR file.\n;\n";
-  AuxModule.get()->print(errs(), nullptr,
- /*ShouldPreserveUseListOrder=*/false,
- /*IsForDebug=*/true);
-  errs() << '\n';
-}
-
-// Find clang in order to create the bundle binary.
-StringRef Dir = sys::path::parent_path(BundlerExecutable);
-
-auto ClangBinary = sys::findProgramByName("clang", Dir);
-if (ClangBinary.getError()) {
-  // Remove bitcode file.
-  sys::fs::remove(BitcodeFileName);
-
-  errs() << "error: unable to find 'clang' in path.\n";
-  return true;
-}
-
-// Do the incremental linking. W

[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 217232.
sdmitriev added a comment.

Rebase.


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,13 +285,14 @@
   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 &Input) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
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,13 +285,14 @@
   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 &Input) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
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] 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 217234.
sdmitriev added a comment.

Rebase.


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
@@ -739,7 +739,6 @@
 
   // Read all the bundles that are in the work list. If we find no bundles we
   // assume the file is meant for the host target.
-  bool FoundHostBundle = false;
   while (!Worklist.empty()) {
 StringRef CurTriple = FH.get()->ReadBundleStart(Input);
 
@@ -765,10 +764,6 @@
 FH.get()->ReadBundle(OutputFile, Input);
 FH.get()->ReadBundleEnd(Input);
 Worklist.erase(Output);
-
-// Record if we found the host bundle.
-if (hasHostKind(CurTriple))
-  FoundHostBundle = true;
   }
 
   // If no bundles were found, assume the input file is the host bundle and
@@ -790,12 +785,6 @@
 return false;
   }
 
-  // If we found elements, we emit an error if none of those were for the host.
-  if (!FoundHostBundle) {
-errs() << "error: Can't find bundle for the host target\n";
-return true;
-  }
-
   // If we still have any elements in the worklist, create empty files for 
them.
   for (auto &E : Worklist) {
 std::error_code EC;
@@ -894,7 +883,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
@@ -183,6 +183,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=s 
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 
-inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
 //
 // Check binary bundle/unbundle. The content that we have before bundling must 
be the same we have after unbundling.
 //
@@ -221,6 +225,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// 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
+
 //
 // 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
@@ -253,6 +261,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=o 
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 
-inputs=%t.bundle3.o -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
 // Some code so that we can create a binary out of this file.
 int A = 0;
 void test_func(void) {


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -739,7 +739,6 @@
 
   // Read all the bundles that are in the work list. If we find no bundles we
   // assume the file is meant for the host target.
-  bool FoundHostBundle = false;
   while (!Worklist.empty()) {
 StringRef CurTriple = FH.get()->ReadBundleStart(Input);
 
@@ -765,10 +764,6 @@
 FH.get()->ReadBundle(OutputFile, Input);
 FH.get()->ReadBundleEnd(Input);
 Worklist.erase(Output);
-
-// Record if we found the host bundle.
-if (hasHostKind(CurTriple))
-  FoundHostBundle = true;
   }
 
   // If no bundles were found, assume the input file is the host bundle and
@@ -790,12 +785,6 @@
 return false;
   }
 
-  // If we found elements, we emit an error if none of those were for the host.
-  if (!FoundHostBundle) {
-errs() << "error: Can't find bundle for the host target\n";
-return true;
-  }
-
   // If we still have any elements in the worklist, create empty files for them.
   for (auto &E : Worklist) {
 std::error_code EC;
@@ -894,7 +883,9 @@
 ++Index;
   }
 
-  if (HostTargetNum != 1) {
+  // Host triple is not really needed for unbundling operation, so do not
+  // treat missing host triple 

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


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;

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?



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

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
@@ -790,8 +790,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;
   }
@@ -894,7 +895,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
@@ -183,6 +183,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=s 
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 
-inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
 //
 // Check binary bundle/unbundle. The content that we have before bundling must 
be the same we have after unbundling.
 //
@@ -221,6 +225,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// 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
+
 //
 // 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
@@ -253,6 +261,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=o 
-targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 
-inputs=%t.bundle3.o -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
 // Some code so that we can create a binary out of this file.
 int A = 0;
 void test_func(void) {


Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -790,8 +790,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;
   }
@@ -894,7 +895,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
@@ -183,6 +183,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=s -targets=openmp-powerpc64le-ibm-linux-gnu -outputs=%t.res.tgt1 -inputs=%t.bundle3.s -unbundle
+// RUN: diff %t.tgt1 %t.res.tgt1
+
 //
 // Check binary bundle/unbundle. The content that we have before bundling must be the same we have after unbundling.
 //
@@ -221,6 +225,10 @@
 // RUN: diff %t.empty %t.res.tgt1
 // RUN: diff %t.empty %t.res.tgt2
 
+// Check that we can extract target parts without providing host triple.
+// RUN: clang-offload-bundler -type=ast -targets=openmp-powerpc64le-ibm-l

[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

[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/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/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] 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] 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 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 &Input) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
   void ReadBundleEnd(MemoryBuffer &Input) final {
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
-++CurBundleInfo;
   }
 
   void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) 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 &Input) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
   void ReadBundleEnd(MemoryBuffer &Input) final {
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
-++CurBundleInfo;
   }
 
   void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) 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:295
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();

ABataev wrote:
> Maybe just:
> ```
> const BundleInfo &Bundle = *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]

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&id=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 &Input) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
   void ReadBundleEnd(MemoryBuffer &Input) final {
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
-++CurBundleInfo;
   }
 
   void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) 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 &Input) final {
-if (CurBundleInfo == BundlesInfo.end())
+if (NextBundleInfo == BundlesInfo.end())
   return StringRef();
-
+CurBundleInfo = NextBundleInfo++;
 return CurBundleInfo->first();
   }
 
   void ReadBundleEnd(MemoryBuffer &Input) final {
 assert(CurBundleInfo != BundlesInfo.end() && "Invalid reader info!");
-++CurBundleInfo;
   }
 
   void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) final {
___
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] 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]

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&id=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 -targets=host-%itanium_abi_triple,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-l

[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] 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(), getEnt

[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 &OffloadKind,
@@ -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 &OS, StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
@@ -327,7 +332,7 @@
 
 unsigned Idx = 0;
 for (auto &T : TargetNames) {
-  MemoryBuffer &MB = *Inputs[Idx++].get();
+  MemoryBuffer &MB = *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;
   }
 
@@ -6

[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 &OffloadKind,
@@ -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 &OS, StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
@@ -327,7 +339,7 @@
 
 unsigned Idx = 0;
 for (auto &T : TargetNames) {
-  MemoryBuffer &MB = *Inputs[Idx++].get();
+  MemoryBuffer &MB = *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,
+"'" + 

[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 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 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 &Input) = 0;
+  virtual Error ReadHeader(MemoryBuffer &Input) = 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 &Input) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer &Input) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer &Input) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer &Input) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 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 &OS,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream &OS,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream &OS,
+ 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 &OS, StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -222,7 +224,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer &Input) final {
+  Error ReadHeader(MemoryBuffer &Input) 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;

[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 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 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 &Input) = 0;
+  virtual Error ReadHeader(MemoryBuffer &Input) = 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 &Input) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer &Input) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer &Input) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer &Input) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 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 &OS,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream &OS,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream &OS,
+ 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 &OS, StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -222,7 +226,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer &Input) final {
+  Error ReadHeader(MemoryBuffer &Input) 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

[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 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 &Input) = 0;
+  virtual Error ReadHeader(MemoryBuffer &Input) = 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 &Input) = 0;
+  virtual Expected>
+  ReadBundleStart(MemoryBuffer &Input) = 0;
 
   /// Read the marker that closes the current bundle.
-  virtual void ReadBundleEnd(MemoryBuffer &Input) = 0;
+  virtual Error ReadBundleEnd(MemoryBuffer &Input) = 0;
 
   /// Read the current bundle and write the result into the stream \a OS.
-  virtual void ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error ReadBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 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 &OS,
-   ArrayRef> Inputs) = 0;
+  virtual Error WriteHeader(raw_fd_ostream &OS,
+ArrayRef> Inputs) = 0;
 
   /// Write the marker that initiates a bundle for the triple \a TargetTriple to
   /// \a OS.
-  virtual void WriteBundleStart(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
+  virtual Error WriteBundleStart(raw_fd_ostream &OS,
+ 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 &OS, StringRef TargetTriple) = 0;
+  /// OS.
+  virtual Error WriteBundleEnd(raw_fd_ostream &OS, StringRef TargetTriple) = 0;
 
   /// Write the bundle from \a Input into \a OS.
-  virtual void WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
+  virtual Error WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) = 0;
 };
 
 /// Handler for binary files. The bundled file will have the following format
@@ -222,7 +227,7 @@
 
   ~BinaryFileHandler() final {}
 
-  void ReadHeader(MemoryBuffer &Input) final {
+  Error ReadHeader(MemoryBuffer &Input) 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(F

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

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