[PATCH] D134662: [OpenMPIRBuilder] Migrate emitOffloadingArraysArgument from clang

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Initial feedback, mostly style.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1026
+    /// Indicate whether any user-defined mapper exists.
+  };
+
----------------
No `llvm::` in `llvm/`


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1044
+    //    llvm::DenseMap<const ValueDecl *, Address> CaptureDeviceAddrMap;
+    llvm::DenseMap<const void *, Value*> CaptureDeviceAddrMap; // FIXME
+
----------------
Seems unused. If needed in can live in Clang.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1053-1059
+      RTArgs.BasePointersArray = nullptr;
+      RTArgs.PointersArray = nullptr;
+      RTArgs.SizesArray = nullptr;
+      RTArgs.MapTypesArray = nullptr;
+      RTArgs.MapTypesArrayEnd = nullptr;
+      RTArgs.MapNamesArray = nullptr;
+      RTArgs.MappersArray = nullptr;
----------------
RTArgs = TargetDataRTArgs()?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3997-4001
+  auto Int8Ty = Type::getInt8Ty(M.getContext());
+  auto VoidPtrTy =  Int8Ty->getPointerTo(0);
+  auto VoidPtrPtrTy = VoidPtrTy->getPointerTo(0);
+  auto Int64Ty = Type::getInt64Ty(M.getContext());
+  auto Int64PtrTy = Int64Ty->getPointerTo();
----------------
These types should exist but I guess this is OK.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4002
+  auto Int64PtrTy = Int64Ty->getPointerTo();
+  if (Info.NumberOfPtrs) {
+    RTArgs.BasePointersArray = Builder.CreateConstInBoundsGEP2_32(
----------------
Flip the cases, simple first, then exit.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4026
+    // requested.
+    if (EmitDebug)
+      RTArgs.MapNamesArray = ConstantPointerNull::get(VoidPtrPtrTy);
----------------
!EmitDebug


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

https://reviews.llvm.org/D134662