[PATCH] D137587: [OpenMP][OMPIRBuilder] Migrate target outlined function registration to OMPIRBuilder from clang

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1379
+  void setOutlinedFunctionAttributes(Function *OutlinedFn, bool IsEmbedded,
+                                     int32_t NumTeams, int32_t NumThreads);
+
----------------
Outlined function is too generic for the function name here and elsewhere. It's an outlined target region, or similar. We outline a lot of functions but this is specific to target regions, correct?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3960
+    OutlinedFn->setLinkage(GlobalValue::WeakODRLinkage);
+    OutlinedFn->setDSOLocal(false);
+    OutlinedFn->setVisibility(GlobalValue::ProtectedVisibility);
----------------
This is not DSO local? Maybe add a TODO to check this in the future.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3979-3984
+  } else {
+    return new GlobalVariable(M, Builder.getInt8Ty(), /*isConstant=*/true,
+                              GlobalValue::WeakAnyLinkage,
+                              Constant::getNullValue(Builder.getInt8Ty()),
+                              EntryFnIDName);
+  }
----------------
no else after return


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

https://reviews.llvm.org/D137587