[PATCH] D136853: [OpenMP][OMPIRBuilder] Migrate createOffloadEntriesAndInfoMetadata from clang to OpenMPIRBuilder

Thu Oct 27 11:09:43 PDT 2022

jdoerfert added a comment.

I think the pure code move looks good. My comments below are more concerned with cleanup/clarification we could do while we are here.

================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:27
 class CanonicalLoopInfo;
+class TargetRegionEntryInfo;
 class OffloadEntriesInfoManager;
----------------
It's a struct, according to the pre-commit buildbot error.

================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1101
+                          uint64_t Size, int32_t Flags,
+                          GlobalValue::LinkageTypes);
+
----------------
IsGPU is not great. If anything, let's call it "IsTargetCodegen" or similar. Also elsewhere. As this clashes with the special handling of "host offloading", I would suggest to then rename `IsDevice` to `IsEmbedded` or something. This should work for non-GPUs and also the "IsDevice" flag is misnamed as the host offloading is also targeting a device. 

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4734
+  if (IsSIMD || OffloadEntriesInfoManager.empty())
+    return;
+
----------------
Why do we call it in SIMD mode in the first place?
The Module should be available to the IRBuilder. 

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

https://reviews.llvm.org/D136853