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

[Sergey Dmitriev via Phabricator via Openmp-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<std::unique_ptr<MemoryBuffer>>;
+
----------------
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