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

[Jan Sjödin via Phabricator via llvm-commits]

jsjodin marked an inline comment as not done.
jsjodin added a comment.

The tests are unaffected since this is just refactoring and migration.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9752
+      return;
+    }
+
----------------
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.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:442
+
+  SmallVector<Value *> getKernelArgsVector(IRBuilderBase &Builder);
+
----------------
jdoerfert wrote:
> At least move it out.
Not sure exactly what you meant, but I moved this struct inside the OMPIRBuilder since it makes sense for it to use the runtime args struct.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:336
+TargetKernelArgs::getKernelArgsVector(IRBuilderBase &Builder) {
+  Value *Version = Builder.getInt32(/* Version */ 2);
+  Value *PointerNum = Builder.getInt32(NumTargetItems);
----------------
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?



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

https://reviews.llvm.org/D151035