[PATCH] D80676: [mlir][gpu] Introduce mlir-rocm-runner.

[Wen-Heng (Jack) Chung via Phabricator via llvm-commits]

whchung marked 7 inline comments as done.
whchung added inline comments.


================
Comment at: mlir/tools/mlir-rocm-runner/mlir-rocm-runner.cpp:71
+                                       cl::value_desc("AMDGPU ISA version"),
+                                       cl::init("gfx900"));
+
----------------
jerryyin wrote:
> Adding a TODO regarding on supporting other target chips?
Other targets such as `gfx906` `gfx908` can already work via this option. I'll add a TODO to detect native AMD GPU ISA version on the system.


================
Comment at: mlir/tools/mlir-rocm-runner/rocm-runtime-wrappers.cpp:122
+template <typename T>
+void mgpuMemGetDevicePointer(T *hostPtr, T **devicePtr) {
+  reportErrorIfAny(hipSetDevice(0), "hipSetDevice");
----------------
jerryyin wrote:
> I'm a bit concerned that we get diverged between different user APIs. It is probably fine now, but will be a maintenance burden later. I will have to, in the future, duplicate and sync every cuda-rocm-runner test with us. 
> 
> Desirably, we can reach to a shape where cuda-runner and rocm-runner tests can be merged into one.
This only has to deal with host pinned memrefs. In real-world applications memrefs could be explicitly allocated on GPU, and transferred between CPU and GPU.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80676