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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits