[PATCH] D151035: [OpenMP][OpenMPIRBuilder] Migrate kernel launch code and host fallback code generation from Clang to the OpenMPIRBuilder

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Looks generally fine. Some minor nits. Also, this does not impact any tests?



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9588
+  if (OffloadingMandatory) {
+    CGF.Builder.CreateUnreachable();
+  } else {
----------------
Unrelated but I feel we should at least emit a trap. This feels user hostile and there is nothing we can really optimize here.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9752
+      return;
+    }
+
----------------
I don't get this code. If we are reverse offloading we are not (necessarily) on the host, right? 
We should not do "something" for things we have not implemented, IMHO. Assert out and ensure SEMA emits an error.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9760
+    llvm::Value *sizesArray = InputInfo.SizesArray.getPointer();
+    llvm::Value *mappersArray = InputInfo.MappersArray.getPointer();
+
----------------
Style, above and below.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:442
+
+  SmallVector<Value *> getKernelArgsVector(IRBuilderBase &Builder);
+
----------------
At least move it out.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1409
 
+  using EmitFallbackCallbackTy = function_ref<InsertPointTy(InsertPointTy)>;
+
----------------
Docs.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:336
+TargetKernelArgs::getKernelArgsVector(IRBuilderBase &Builder) {
+  Value *Version = Builder.getInt32(/* Version */ 2);
+  Value *PointerNum = Builder.getInt32(NumTargetItems);
----------------
We should make this a constant variable in some header.


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

https://reviews.llvm.org/D151035