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

[Zhuoran Yin via Phabricator via llvm-commits]

jerryyin accepted this revision.
jerryyin added a comment.
This revision is now accepted and ready to land.

Good job, mostly minor issues, except the one related with the difference between `mlir-rocm-runner` and `mlir-cuda-runner`. Giving an accept to unblock future development, but we need to figure out the best way to resolve the differences between runner wrapper interfaces.



================
Comment at: mlir/tools/mlir-rocm-runner/mlir-rocm-runner.cpp:71
+                                       cl::value_desc("AMDGPU ISA version"),
+                                       cl::init("gfx900"));
+
----------------
Adding a TODO regarding on supporting other target chips?


================
Comment at: mlir/tools/mlir-rocm-runner/mlir-rocm-runner.cpp:80
+  raw_svector_ostream os(result);
+  std::unique_ptr<buffer_ostream> bos = std::make_unique<buffer_ostream>(os);
+
----------------
I only see you use the raw os stream, not the bos. Should you swap os with bos in the lines follows?


================
Comment at: mlir/tools/mlir-rocm-runner/mlir-rocm-runner.cpp:138
+
+static LogicalResult createHsaco(Blob &isaBlob, StringRef name,
+                                 Blob &hsacoBlob) {
----------------
Nit: Could you add a const to &isaBlob just so that it is clear it is input


================
Comment at: mlir/tools/mlir-rocm-runner/mlir-rocm-runner.cpp:190
+  }
+  ec = sys::fs::remove(temphsacoFilename);
+  if (ec) {
----------------
Nit: Use the stack allocated `llvm::FileRemover` instead of having to manually track/remove it?


================
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");
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80676