[Openmp-commits] [PATCH] D71384: [libomptarget] Implement hsa plugin

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/plugins/hsa/CMakeLists.txt:134
+# We need the AOMP specific build of ATMI that has HSA_INTEROP turned on. 
+# Also, the AOMP specific build of ATMI has seperate release and debug builds. 
+target_link_libraries(
----------------
What are all the reasons we need AOMP here and not the LLVM/Clang we ship this with?


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:12
+//===----------------------------------------------------------------------===//
+
+#include <algorithm>
----------------
JonChesterfield wrote:
> jdoerfert wrote:
> > JonChesterfield wrote:
> > > These headers are part of why it isn't enabled by default - I'm not totally sure which dev libraries can be assumed to be installed when building llvm. ffi.h and gelf.h may not be on the list, in which case extra cmake logic or dropping the dependencies would be needed.
> > ffi should already have CMAKE flag: `FFI_INCLUDE_DIR` and `FFI_LIBRARY_DIR`, the other one might also, there is an include of `gelf.h` in `openmp/libomptarget/plugins/generic-elf-64bit/src/rtl.cpp` (the only one in llvm).
> That's good, thanks.
> 
> I think the detection logic in the cmake will avoid trying to build this on systems that don't have the atmi library available, in which case enabling this by default will be safe.
I would try it on such a system if possible ;)


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:162
+  // static int EnvNumThreads;
+  static const int HardTeamLimit = 1 << 20; // 1 Meg
+  static const int DefaultNumTeams = 128;
----------------
For the record, no need to fix this now: As with the team limit env variable (see below), I think this has to be handled differently in 5.1. We need to deal with arbitrary team sizes, if the hardware doesn't we need to have a software loop.


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:620
+  // https://github.com/RadeonOpenCompute/atmi-staging/issues/67
+  // https://github.com/RadeonOpenCompute/atmi-staging/issues/65
+
----------------
Temporary fixes are the most enduring ones usually.


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:655
+  __tgt_offload_entry *HostBegin = image->EntriesBegin;
+  __tgt_offload_entry *HostEnd = image->EntriesEnd;
+
----------------
Assuming this is the same for all targets, I would think this stuff should live in a plugins/helper file. I mean, a way to iterate over all offload entries.  What do you think?


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:1180
+        num_groups > DeviceInfo.EnvMaxTeamsDefault)
+      num_groups = DeviceInfo.EnvMaxTeamsDefault;
+  }
----------------
I'm fairly certain we have to get rid of this `EnvMaxTeamsDefault`. Btw. I cannot find it in my llvm version.


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:1186
+    fprintf(stderr, "loop_tripcount: %ld\n", loop_tripcount);
+  }
+  DP("Final %d num_groups and %d threadsPerGroup\n", num_groups,
----------------
Can we have a macro for this `if (print_kernel_trace > 1) { fprint ... }` scheme? Less control flow will make this easier to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71384