[mlir][spirv] Implement gpu::TargetAttrInterface #69949

silee2 · 2023-10-23T17:34:28Z

Create SPIRV Target Attribute to enable GPU compilation pipeline. The Target Attribute is modeled after the existing spriv.target_env Plan is to use this new attribute to enable GPU compilation pipeline for OpenCL kernels only.
The changes do not impact Vulkan shaders using milr-vulkan-runner. New GPU Dialect transform pass spirv-attach-target is implemented for attaching attribute from CLI.
gpu-module-to-binary pass now works with GPU module that has SPIRV module with OpenCL kernel functions inside.

Create SPIRV Target Attribute to enable GPU compilation pipeline. The Target Attribute is modeled after the existing spriv.target_env Plan is to use this new attribute to enable GPU compilation pipeline for OpenCL kernels only. The changes do not impact Vulkan shaders using milr-vulkan-runner. New GPU Dialect transform pass spirv-attach-target is implemented for attaching attribute from CLI. gpu-module-to-binary pass now works with GPU module that has SPIRV module with OpenCL kernel functions inside.

silee2 · 2023-10-23T17:35:35Z

Part of refactoring and breaking down #65539 into smaller PRs.

silee2 · 2023-10-23T18:44:30Z

@fabianmcg Let me know if I'm missing anything. I have one more PR after this one that makes target LLVMIR aware of SPIRV dialect since the new target attribute is part of SPIRV dialect which was not considered a leaf dialect that needs translation to LLVMIR.

fabianmcg · 2023-10-23T19:10:40Z

@silee2 I'll take a closer look later, at first glance it looks like everything is there.

Just one quick question, can you describe the next PR a bit more? Because you shouldn't need to make the target LLVMIR aware, as the binary embedding is mostly dialect agnostic, so there shouldn't be an issue with SPIRVTargetAttr not having a translation to LLVM.
From what I remember from the original PR the only other modification needed is changing how the kernel is launched, and that change must be done in Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp

fabianmcg · 2023-10-24T13:36:40Z

mlir/lib/Target/SPIRV/Target.cpp

+  }
+  auto gpuMod = dyn_cast<gpu::GPUModuleOp>(module);
+  auto spvMods = gpuMod.getOps<spirv::ModuleOp>();
+  auto spvMod = *spvMods.begin();


A check is necessary before *spvMods.begin(), as spvMods could be an empty range.

Added check for empty range.

fabianmcg · 2023-10-24T13:36:45Z

mlir/lib/Target/SPIRV/Target.cpp

+    return std::nullopt;
+  }
+
+  SmallVector<char, 0> spvData;


I'd suggest instead of the loop:

Suggested change

SmallVector<char, 0> spvData;

SmallVector<char, 0> spvData(spvBinary.size() * sizeof(uint32_t), 0);

memcpy(spvData.data(), spvBinary.data(), spvData.size());

Changed as suggested.

fabianmcg · 2023-10-24T13:53:01Z

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVAttributes.td

+  let description = [{
+    GPU target attribute for controlling compilation of SPIRV targets.
+  }];
+  let parameters = (ins


What is the purpose of all of these parameters? I couldn't find the place where they are being used, are they being used at a later patch, or are they only informational?

Those parameters are for describing target environment: https://mlir.llvm.org/docs/Dialects/SPIR-V/#target-environment
The parameters are not used today since lowering to SPIR-V is restricted to common set of SPIR-V ops supported in all environments.
But in the future, lowering passes can check the parameters and utilize SPIR-V extension ops or do target specific optimizations.

silee2 · 2023-10-26T18:08:03Z

@silee2 I'll take a closer look later, at first glance it looks like everything is there.

Just one quick question, can you describe the next PR a bit more? Because you shouldn't need to make the target LLVMIR aware, as the binary embedding is mostly dialect agnostic, so there shouldn't be an issue with SPIRVTargetAttr not having a translation to LLVM. From what I remember from the original PR the only other modification needed is changing how the kernel is launched, and that change must be done in Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp

silee2 · 2023-10-26T18:19:32Z

@silee2 I'll take a closer look later, at first glance it looks like everything is there.

Just one quick question, can you describe the next PR a bit more? Because you shouldn't need to make the target LLVMIR aware, as the binary embedding is mostly dialect agnostic, so there shouldn't be an issue with SPIRVTargetAttr not having a translation to LLVM. From what I remember from the original PR the only other modification needed is changing how the kernel is launched, and that change must be done in Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp

Yes. SelectObjectAttr.cpp is the only functional change.
But the new target attribute "spirv.target" is part of SPIRV dialect.
And SPIRV dialect becomes a new dialect as part of input to LLVMIR translation.
The additional change updates
mlir/include/mlir/Target/LLVMIR/Dialect/All.h
which now includes
registerSPIRVDialectTranslation(registry);
There are no actual SPIRV ops to translate and just a single attribute type.
So the new files
mlir/include/mlir/Target/LLVMIR/Dialect/SPIRV/SPIRVToLLVMIRTranslation.h
and
mlir/lib/Target/LLVMIR/Dialect/SPIRV/SPIRVToLLVMIRTranslation.cpp
are mostly empty.
Without those changes, mlir-cpu-runner will see spirv.target attribute in the input which is part of SPIRV dialect. And since SPIRV dialect is not registered, mlir-cpu-runner throws an error.
NVVM and ROCDL did not have this issue since NVVM and ROCDL was already registered as an input dialect for LLVMIR translation.

silee2 · 2023-10-26T18:58:17Z

@fabianmcg Can you take a look again? Answered question and addressed change requests.

fabianmcg · 2023-10-26T19:17:10Z

This patch LGTM! I'll approve once the builds have completed.

Without those changes, mlir-cpu-runner will see spirv.target attribute in the input which is part of SPIRV dialect. And since SPIRV dialect is not registered, mlir-cpu-runner throws an error.

Did you try just registering the dialect instead of adding a translation? I'm still unsure if adding a translation to LLVM is needed, as the error is probably only happening during parsing? But I guess we can have this conversation on the next PR.

silee2 · 2023-10-26T21:36:45Z

check-flang failed on Windows with two unresolved tests. Does not look related to this PR. Merging main again to run CI again.

silee2 · 2023-10-27T16:46:51Z

@joker-eph This PR got approval from @fabianmcg. What do I need to do next to get the PR merged?

antiagainst · 2023-10-27T23:20:19Z

Thanks for the contribution and sorry about the delay! I'm getting around to review this pull request (already done a few others). Please wait a bit.

antiagainst

Thanks again for the contribution! A couple of comments inlined. My main concern is the duplication of spirv.target attribute. We should avoid the duplication and confusion there.

antiagainst · 2023-10-28T23:43:44Z

mlir/include/mlir/Dialect/GPU/Transforms/Passes.td

@@ -188,4 +188,46 @@ def GpuROCDLAttachTarget: Pass<"rocdl-attach-target", ""> {
  ];
 }

+def GpuSPIRVAttachTarget: Pass<"spirv-attach-target", ""> {
+  let summary = "Attaches an SPIRV target attribute to a GPU Module.";


Nit: s/SPIRV/SPIR-V/.

SPIR-V is the formal spelling. We should use that when in docs and comments.

Also for all the following docs and comments.

antiagainst · 2023-10-28T23:52:55Z

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVAttributes.td

+// SPIRV target attribute.
+//===----------------------------------------------------------------------===//
+
+def SPIRV_TargetAttr : SPIRV_Attr<"SPIRVTarget", "target"> {


This attribute is pretty much a duplication of spirv::TargetEnvAttr defined in SPIRVAttributes.h? We should avoid introducing such duplications and causing confusion. Any reaons we cannot use spirv::TargetEnvAttr directly?

Updated the PR to use spirv::TargetEnvAttr.

antiagainst · 2023-10-28T23:54:17Z

mlir/include/mlir/Target/SPIRV/Target.h

@@ -0,0 +1,30 @@
+//===- Target.h - MLIR SPIRV target registration ----------------*- C++ -*-===//


Nit: again, please use SPIR-V in docs and comments.

antiagainst · 2023-10-28T23:54:37Z

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVAttributes.td

@@ -166,4 +166,35 @@ def SPIRV_ResourceLimitsAttr : SPIRV_Attr<"ResourceLimits", "resource_limits"> {
  let assemblyFormat = "`<` struct(params) `>`";
 }

+//===----------------------------------------------------------------------===//
+// SPIRV target attribute.


Nit: SPIR-V

antiagainst · 2023-10-28T23:57:43Z

mlir/lib/Dialect/GPU/Transforms/SPIRVAttachTarget.cpp

+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements the `GpuSPIRVAttachTarget` pass, attaching


Nit: GPUSPIRVAttachTarget

antiagainst · 2023-10-29T00:20:02Z

mlir/lib/Target/SPIRV/Target.cpp

+    module->emitError("Module must be a GPU module.");
+    return std::nullopt;
+  }
+  auto gpuMod = dyn_cast<gpu::GPUModuleOp>(module);


We could move this upward and check whether it's null instead of the isa check at L77.

antiagainst · 2023-10-29T00:20:29Z

mlir/lib/Target/SPIRV/Target.cpp

+  auto gpuMod = dyn_cast<gpu::GPUModuleOp>(module);
+  auto spvMods = gpuMod.getOps<spirv::ModuleOp>();
+  // Empty spirv::ModuleOp
+  if (spvMods.empty()) {


Nit: drop {..} for trivial if statements per LLVM coding sytle.

antiagainst · 2023-10-29T00:21:27Z

mlir/lib/Target/SPIRV/Target.cpp

+  llvm::SmallVector<uint32_t, 0> spvBinary;
+
+  spvBinary.clear();
+  // serialize the spv module to spv binary


Serialize the spv.module op to SPIR-V blob.

antiagainst · 2023-10-29T00:23:11Z

mlir/lib/Target/SPIRV/Target.cpp

+  spvBinary.clear();
+  // serialize the spv module to spv binary
+  if (mlir::failed(spirv::serialize(spvMod, spvBinary))) {
+    spvMod.emitError() << "Failed to serialize SPIR-V module";


Nit: "failed to .."

no capitalization to compose better with previous / following messages.

Done. Thanks for the tip.

antiagainst · 2023-10-29T00:26:51Z

mlir/lib/Target/SPIRV/Target.cpp

+  }
+  auto gpuMod = dyn_cast<gpu::GPUModuleOp>(module);
+  auto spvMods = gpuMod.getOps<spirv::ModuleOp>();
+  // Empty spirv::ModuleOp


The comment here does not provide additional value; we should remove it or expand it to be more clear.

silee2 · 2023-10-30T21:26:54Z

@antiagainst Thanks for the detailed review and helpful comments! Updated the PR accordingly.

silee2 · 2023-11-03T18:42:35Z

@antiagainst Waiting for your approval.

antiagainst

LGTM. Thanks for addressing the comments!

antiagainst · 2023-11-05T16:12:24Z

I revised the commit message a bit and landed it. Thanks for the waiting!

JOE1994 · 2023-11-05T18:21:06Z

This revision seems related to the following buildbot failure:

https://lab.llvm.org/buildbot/#/builders/268/builds/1747
(+ builds 1748 & 1749)

Would you mind taking a look? Thank you 👍

antiagainst · 2023-11-05T18:45:27Z

Sorry about that! I uploaded 4a4b857 to fix this. Verified that it fixes the break for me locally.

jpienaar · 2023-11-05T19:34:15Z

mlir/include/mlir/Dialect/GPU/Transforms/Passes.h

@@ -15,6 +15,7 @@

 #include "Utils.h"
 #include "mlir/Dialect/GPU/IR/GPUDialect.h"
+#include "mlir/Dialect/SPIRV/IR/SPIRVAttributes.h"


This feels off: you have GPU dialect transforms depending on SPIRV attributes/dialect (esp in header). Why is this pass in GPU dialect rather than SPIRV one?

Back when the compilation redesign was happening we decided to add the attach* passes in GPU to avoid polluting lower level dialects with GPU includes. However, I do agree that include shouldn't be there, as far as I could tell, that include is only needed by one pass option mlir::spirv::TargetEnvAttr::kUnknownDeviceID, so it should be possible to remove it. I'll fix it.

SG. Thanks @jpienaar & @fabianmcg!

llvmbot added mlir:gpu mlir:spirv mlir labels Oct 23, 2023

Register spirv target attribute.

54adf40

fabianmcg self-requested a review October 23, 2023 19:15

fabianmcg requested changes Oct 24, 2023

View reviewed changes

silee2 closed this Oct 26, 2023

silee2 reopened this Oct 26, 2023

Address reviewer comments.

08b1d2d

Merge branch 'main' into spirv_target_attr

aecb8be

fabianmcg approved these changes Oct 26, 2023

View reviewed changes

antiagainst requested review from antiagainst and kuhar October 27, 2023 23:20

antiagainst requested changes Oct 29, 2023

View reviewed changes

silee2 added 3 commits October 30, 2023 19:41

Merge remote-tracking branch 'origin/main' into spirv_target_attr_rework

f63e385

Use existing TargetEnvAttr instead of introducing a new Attribute.

7f0f119

Address reviewer comments.

6a9e268

antiagainst approved these changes Nov 5, 2023

View reviewed changes

antiagainst changed the title ~~[MLIR] SPIRV Target Attribute~~ [mlir][spirv] Implement gpu::TargetAttrInterface Nov 5, 2023

antiagainst merged commit 2dace04 into llvm:main Nov 5, 2023

jpienaar reviewed Nov 5, 2023

View reviewed changes

	SmallVector<char, 0> spvData;
	SmallVector<char, 0> spvData(spvBinary.size() * sizeof(uint32_t), 0);
	memcpy(spvData.data(), spvBinary.data(), spvData.size());

		@@ -0,0 +1,30 @@
		//===- Target.h - MLIR SPIRV target registration ----------------- C++ --===//

[mlir][spirv] Implement gpu::TargetAttrInterface #69949

[mlir][spirv] Implement gpu::TargetAttrInterface #69949

Uh oh!

Conversation

silee2 commented Oct 23, 2023

Uh oh!

silee2 commented Oct 23, 2023

Uh oh!

silee2 commented Oct 23, 2023

Uh oh!

fabianmcg commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silee2 commented Oct 26, 2023

Uh oh!

silee2 commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silee2 commented Oct 26, 2023

Uh oh!

fabianmcg commented Oct 26, 2023

Uh oh!

silee2 commented Oct 26, 2023

Uh oh!

silee2 commented Oct 27, 2023

Uh oh!

antiagainst commented Oct 27, 2023

Uh oh!

antiagainst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fabianmcg commented Oct 23, 2023 •

edited

Loading

silee2 commented Oct 26, 2023 •

edited

Loading

silee2 commented Nov 3, 2023 •

edited

Loading