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

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:69
+
+  using MemoryBuffersVector = SmallVectorImpl<std::unique_ptr<MemoryBuffer>>;
+
----------------
Why not `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);
+  }
----------------
Not sure that this is the best solution, we may end up with incorrect `size_t` type in some cases. 


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:73
+  IntegerType *getSizeTTy() {
+    auto PtrSize = M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C));
+    return PtrSize == 8 ? Type::getInt64Ty(C) : Type::getInt32Ty(C);
----------------
Use real type here, not `auto`


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:133
+  GlobalVariable *createBinDesc(const MemoryBuffersVector &Bufs) {
+    auto *EntriesB = new GlobalVariable(M, getEntryTy(), true,
+                                        GlobalValue::ExternalLinkage, nullptr,
----------------
Add comment for the `true` constant with the name of parameter.


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:136
+                                        "__omp_offloading_entries_begin");
+    auto *EntriesE = new GlobalVariable(M, getEntryTy(), true,
+                                        GlobalValue::ExternalLinkage, nullptr,
----------------
Same here


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:143
+
+    SmallVector<Constant *, 4> ImagesInits;
+    for (const auto &Buf : Bufs) {
----------------
Comments?


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:144
+    SmallVector<Constant *, 4> ImagesInits;
+    for (const auto &Buf : Bufs) {
+      auto *Data = ConstantDataArray::get(
----------------
Use real type, not `auto`


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:148
+
+      auto *Image = new GlobalVariable(M, Data->getType(), true,
+                                          GlobalVariable::InternalLinkage,
----------------
Seems to me the code is not formatted


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:196
+                                        { getBinDescPtrTy() }, false);
+    auto RegFuncC = M.getOrInsertFunction("__tgt_register_lib",
+                                          RegFuncTy);
----------------
Use real type, not `auto`


================
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,
----------------
`llvm::None` instead of `{}`


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:216
+    auto *UnRegFuncTy = FunctionType::get(Type::getVoidTy(C),
+                                          { getBinDescPtrTy() }, false);
+    auto UnRegFuncC = M.getOrInsertFunction("__tgt_unregister_lib",
----------------
Remove extra braces here, they are not needed.


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:222
+    IRBuilder<> Builder(BasicBlock::Create(C, "entry", Func));
+    Builder.CreateCall(UnRegFuncC, { BinDesc });
+    Builder.CreateRetVoid();
----------------
Extra braces


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

https://reviews.llvm.org/D64943