[PATCH] D75192: [mlir][vulkan-runner] Update mlir-vulkan-runner execution driver.

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/lib/Conversion/GPUToVulkan/ConvertGPULaunchFuncToVulkanLaunchFunc.cpp:36
+
+// A pass to convert gpu launch op to vulkan launch call op, by creating a
+// SPIR-V binary shader from `spirv::ModuleOp` using `spirv::serialize`
----------------
Use /// for top level comments.


================
Comment at: mlir/lib/Conversion/GPUToVulkan/ConvertGPULaunchFuncToVulkanLaunchFunc.cpp:119
+  SmallVector<uint32_t, 0> binary;
+  for (auto spirvModule : module.getOps<spirv::ModuleOp>()) {
+    if (done)
----------------
Why not check the range for `empty() || hasSingleElement()` before doing anything? If there can only be one, we don't need a loop.


================
Comment at: mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp:131
+    // 4. Stride in dim 0.
+    return {operands[numConfigOps + index * loweredMemRefNumOps1D],
+            operands[numConfigOps + index * loweredMemRefNumOps1D + 3]};
----------------
This breaks on GCC5:
https://buildkite.com/mlir/mlir-core/builds/3336#3ba0b41b-c8d6-4ce6-9788-dc12c51a4024


================
Comment at: mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp:161
+    LLVM::CallOp vulkanLaunchCallOp, Value vulkanRuntime) {
+  if (vulkanLaunchCallOp.getNumOperands() == 6)
+    return;
----------------
Can we have a comment? Why 6?


================
Comment at: mlir/tools/mlir-vulkan-runner/VulkanRuntime.h:25
 
-#include <vulkan/vulkan.h>
+#include <vulkan/vulkan.h> // NOLINT
 
----------------
Let's remove this.


================
Comment at: mlir/tools/mlir-vulkan-runner/vulkan-runtime-wrappers.cpp:66
 extern "C" {
-/// Fills the given memref with the given value.
+// Initializes `VulkanRuntimeManager` and returns a pointer to it.
+void *initVulkan() { return new VulkanRuntimeManager(); }
----------------
Use /// for top-level comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75192