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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D64943#1619958 <https://reviews.llvm.org/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.



================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:72
+private:
+  IntegerType *getSizeTTy() {
+    switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) {
----------------
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)`?


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:135
+
+  // Creates binary descriptor for the given device images. Binary descriptor is
+  // an object that is passed to the offloading runtime at program startup and
----------------
Use `\\\` style for comments here


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


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:330
+  BufferRefs.reserve(Inputs.size());
+  for (const auto &File : Inputs) {
+    auto BufOrErr = MemoryBuffer::getFileOrSTDIN(File);
----------------
Use `std:string` instead of `auto`


================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:331
+  for (const auto &File : Inputs) {
+    auto BufOrErr = MemoryBuffer::getFileOrSTDIN(File);
+    if (!BufOrErr) {
----------------
Also, better to specify type expplicitly.


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

https://reviews.llvm.org/D64943