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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield marked an inline comment as done.
JonChesterfield added a comment.

Copying this file into the trunk source tree builds for me (using parts of a local aomp build)

A fprintf simplification is at https://github.com/ROCm-Developer-Tools/llvm-project/pull/63, I'll update this diff once that lands.



================
Comment at: openmp/libomptarget/plugins/hsa/src/rtl.cpp:1180
+        num_groups > DeviceInfo.EnvMaxTeamsDefault)
+      num_groups = DeviceInfo.EnvMaxTeamsDefault;
+  }
----------------
JonChesterfield wrote:
> 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.
OK, so this one isn't a constant - it's another hook for pulling a value from the environment. Cuda/rtl.cpp does a lot of this too.

Comparing the two, both use a class named RTLDeviceInfoTy, but with different fields and different implementations. The cuda and hsa implementations look broadly similar in general - same functions, doing roughly the same things - but there's a lot of differences in the detail. I don't think the code size savings from pulling out common sequences would be worth coupling the two together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71384