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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield marked 7 inline comments as done.
JonChesterfield added a comment.

Nice feedback, thanks!



================
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(
----------------
jdoerfert wrote:
> What are all the reasons we need AOMP here and not the LLVM/Clang we ship this with?
The fundamental reason is the library depends on an ATMI library which ships with aomp, and I don't have a great handle on under what conditions one can bundle runtime libraries with llvm.

Some incidental reasons are that aomp clang has patches related to making OpenMP work on amdgcn that aren't upsteam, e.g. some constant handling that is partly inlined here.


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:12
+//===----------------------------------------------------------------------===//
+
+#include <algorithm>
----------------
jdoerfert wrote:
> 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 ;)
Yes, will definitely do so. I have an intel/nvidia system behind a ssh server that is currently misbehaving, but that's a transient problem.


================
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;
----------------
jdoerfert wrote:
> 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.
Do you happen to know what arbitrary is in this context? UINT32_MAX, UINT64_MAX? It's hopefully documented in the spec.


================
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
+
----------------
jdoerfert wrote:
> Temporary fixes are the most enduring ones usually.
Quite. @ronlieb the links here are dead, do you happen to know what they referred to?


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:655
+  __tgt_offload_entry *HostBegin = image->EntriesBegin;
+  __tgt_offload_entry *HostEnd = image->EntriesEnd;
+
----------------
jdoerfert wrote:
> 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?
Sure - there's probably a number of common patterns between this and some other plugins that should be extracted. This particular one looks OK to me - it's only just shy of iterators.


================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:1180
+        num_groups > DeviceInfo.EnvMaxTeamsDefault)
+      num_groups = DeviceInfo.EnvMaxTeamsDefault;
+  }
----------------
jdoerfert wrote:
> I'm fairly certain we have to get rid of this `EnvMaxTeamsDefault`. Btw. I cannot find it in my llvm version.
Ah, nice catch. The aomp branch has an abstraction over nvptx and amdgcn constants which gets used by clang and this plugin. This is one of the pieces I've missed by not actually compiling against trunk. Will fix.


================
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,
----------------
jdoerfert wrote:
> Can we have a macro for this `if (print_kernel_trace > 1) { fprint ... }` scheme? Less control flow will make this easier to read.
Reluctant to go for a macro but agree with the sentiment. How do you feel about
```
int errprint(const char *fmt, ...) {
  int rc = 0;
  if (print_kernel_trace > 1) {
    va_list args;
    va_start(args, fmt);
    rc = vfprintf(stderr, fmt, args);
    va_end(args);
  }
  return rc;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71384