[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 accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LG, one suggestion below, I doubt it makes an actual difference though, feel free to ignore.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9752
+      return;
+    }
+
----------------
jsjodin wrote:
> jdoerfert wrote:
> > 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.
> The way I understand it is that whatever code that was generated for the device is ignored (since it is wrong) and we only emit the host fallback. I agree this is not the right thing to e.g. it ignores the OMP_TARGET_OFFLOAD environment variable. When refactoring/migrating I avoid changing how the code behaves in case there are tests that rely on this behavior.
Agreed, but please leave a comment. This does not strike me as right.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1598
+  /// vector.
+  static SmallVector<Value *> getKernelArgsVector(TargetKernelArgs &KernelArgs,
+                                                  IRBuilderBase &Builder);
----------------
I meant this.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:336
+TargetKernelArgs::getKernelArgsVector(IRBuilderBase &Builder) {
+  Value *Version = Builder.getInt32(/* Version */ 2);
+  Value *PointerNum = Builder.getInt32(NumTargetItems);
----------------
jsjodin wrote:
> jdoerfert wrote:
> > We should make this a constant variable in some header.
> I put  "#define OMP_KERNEL_ARG_VERSION 2" in OMPConstants.h, is that what you had in mind?
> 
That works.


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

https://reviews.llvm.org/D151035