[Openmp-commits] [PATCH] D93135: [libomptarget][devicertl] Port amdgcn devicertl to openmp

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

The declare target looks good, some stuff seems unrelated though.



================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/src/amdgcn_intrinsics.h:47
+
+#endif
----------------
This seems unrelated, can it go in before or after?


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h:66
+#define EXTERN_SHARED(NAME) __attribute__((shared)) NAME
+#endif
+
----------------
why the enum? Can we move the _OPENMP stuff in a generic header?


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:74
+
+// static
+DEVICE uint32_t SHARED(L1_Barrier);
----------------
Nit: I'd remove the comment.


================
Comment at: openmp/libomptarget/deviceRTLs/common/src/libcall.cu:323
+// Working around here until the compiler quirk is understood
+DEVICE int omp_is_initial_device_OVERLOAD(void) asm("omp_is_initial_device");
+DEVICE int omp_is_initial_device_OVERLOAD(void) {
----------------
JonChesterfield wrote:
> Note to self -  don't use this bodge for nvptx
D38968 is the reason. We should revert that patch as we have context selectors now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93135