[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
This revision was automatically updated to reflect the committed changes.
antiagainst marked an inline comment as done.
Closed by commit rGdf71000d7d5d: [mlir][spirv] Convert linalg.generic for
reduction to SPIR-V ops (authored by antiagainst).
Changed prior to commit:
https://reviews.llvm.org/D73437?vs=241431=241720#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
Files:
mlir/include/mlir/Conversion/LinalgToSPIRV/LinalgToSPIRV.h
mlir/include/mlir/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.h
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h
mlir/include/mlir/Dialect/Linalg/Utils/Utils.h
mlir/include/mlir/Dialect/SPIRV/SPIRVAtomicOps.td
mlir/include/mlir/Dialect/SPIRV/SPIRVControlFlowOps.td
mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h
mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h
mlir/lib/Conversion/CMakeLists.txt
mlir/lib/Conversion/LinalgToSPIRV/CMakeLists.txt
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.cpp
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
mlir/lib/Dialect/Linalg/Utils/Utils.cpp
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
mlir/lib/Dialect/SPIRV/TargetAndABI.cpp
mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
mlir/tools/mlir-opt/CMakeLists.txt
Index: mlir/tools/mlir-opt/CMakeLists.txt
===
--- mlir/tools/mlir-opt/CMakeLists.txt
+++ mlir/tools/mlir-opt/CMakeLists.txt
@@ -41,6 +41,7 @@
MLIRQuantOps
MLIRROCDLIR
MLIRSPIRV
+ MLIRLinalgToSPIRVTransforms
MLIRStandardToSPIRVTransforms
MLIRSPIRVTestPasses
MLIRSPIRVTransforms
Index: mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
===
--- /dev/null
+++ mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
@@ -0,0 +1,162 @@
+// RUN: mlir-opt -split-input-file -convert-linalg-to-spirv -canonicalize -verify-diagnostics %s -o - | FileCheck %s
+
+//===--===//
+// Single workgroup reduction
+//===--===//
+
+#single_workgroup_reduction_trait = {
+ args_in = 1,
+ args_out = 1,
+ iterator_types = ["reduction"],
+ indexing_maps = [
+affine_map<(i) -> (i)>,
+affine_map<(i) -> (0)>
+ ]
+}
+
+module attributes {
+ spv.target_env = {
+version = 3 : i32,
+extensions = [],
+capabilities = [1: i32, 63: i32] // Shader, GroupNonUniformArithmetic
+ }
+} {
+
+// CHECK: spv.globalVariable
+// CHECK-SAME: built_in("LocalInvocationId")
+
+// CHECK: func @single_workgroup_reduction
+// CHECK-SAME: (%[[INPUT:.+]]: !spv.ptr{{.+}}, %[[OUTPUT:.+]]: !spv.ptr{{.+}})
+
+// CHECK:%[[ZERO:.+]] = spv.constant 0 : i32
+// CHECK:%[[ID:.+]] = spv.Load "Input" %{{.+}} : vector<3xi32>
+// CHECK:%[[X:.+]] = spv.CompositeExtract %[[ID]][0 : i32]
+
+// CHECK:%[[INPTR:.+]] = spv.AccessChain %[[INPUT]][%[[ZERO]], %[[X]]]
+// CHECK:%[[VAL:.+]] = spv.Load "StorageBuffer" %[[INPTR]] : i32
+// CHECK:%[[ADD:.+]] = spv.GroupNonUniformIAdd "Subgroup" "Reduce" %[[VAL]] : i32
+
+// CHECK:%[[OUTPTR:.+]] = spv.AccessChain %[[OUTPUT]][%[[ZERO]], %[[ZERO]]]
+// CHECK:%[[ELECT:.+]] = spv.GroupNonUniformElect "Subgroup" : i1
+
+// CHECK:spv.selection {
+// CHECK: spv.BranchConditional %[[ELECT]], ^bb1, ^bb2
+// CHECK:^bb1:
+// CHECK: spv.AtomicIAdd "Device" "AcquireRelease" %[[OUTPTR]], %[[ADD]]
+// CHECK: spv.Branch ^bb2
+// CHECK:^bb2:
+// CHECK: spv._merge
+// CHECK:}
+// CHECK:spv.Return
+
+func @single_workgroup_reduction(%input: memref<16xi32>, %output: memref<1xi32>) attributes {
+ spv.entry_point_abi = {local_size = dense<[16, 1, 1]>: vector<3xi32>}
+} {
+ linalg.generic #single_workgroup_reduction_trait %input, %output {
+^bb(%in: i32, %out: i32):
+ %sum = addi %in, %out : i32
+ linalg.yield %sum : i32
+ } : memref<16xi32>, memref<1xi32>
+ spv.Return
+}
+}
+
+// -
+
+// Missing shader entry point ABI
+
+#single_workgroup_reduction_trait = {
+ args_in = 1,
+ args_out = 1,
+ iterator_types = ["reduction"],
+ indexing_maps = [
+affine_map<(i) -> (i)>,
+affine_map<(i) -> (0)>
+ ]
+}
+
+module attributes {
+ spv.target_env = {
+version = 3 : i32,
+extensions = [],
+capabilities = [1: i32, 63: i32] // Shader, GroupNonUniformArithmetic
+ }
+} {
+func @single_workgroup_reduction(%input: memref<16xi32>, %output: memref<1xi32>) {
+ // expected-error @+1 {{failed to legalize operation 'linalg.generic'}}
+ linalg.generic #single_workgroup_reduction_trait %input, %output {
+^bb(%in: i32,
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
antiagainst marked 4 inline comments as done. antiagainst added inline comments. Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:125 + +PatternMatchResult SingleWorkgroupReduction::matchAndRewrite( +linalg::GenericOp genericOp, ArrayRef operands, nicolasvasilache wrote: > antiagainst wrote: > > nicolasvasilache wrote: > > > Please split this into a precondition and an apply that must succeed so > > > we can use the precondition as a Constraint and the apply as a simple > > > Rewrite. > > > I have found this split to be crucial to write custom fused passes with > > > DRR. > > > I claim this is generally the way we should be writing transformations > > > when we can, which is the case for Linalg ops. > > I feel I don't have all the backgrounds here so I'd appreciate some > > explanation as for why it's necessary to separate the match and rewrite. I > > think that's the original design of patterns (that we have `match` and > > `rewrite` separately) and then later we found it's too cumbersome to carry > > over a large matched state between them and then came up with > > `matchAndRewrite`? IIUC, every pattern is applied as a whole. I can > > understand that the match/rewrite side can be potentially reusable across > > patterns as components if they are exposed as helper functions, but that's > > more from a library organization and code reuse's perspective; I feel > > you've more reasons than that so I'd like to learn about them. :) > > > > Another question here is that I've matches that is generally useful to > > other patterns (checking whether this is a linalg doing reduction) and > > matches that is only useful to SPIR-V here (checking whether the workload > > can be fit in one local workgroup). How do you handle such cases? > > > > For now I separated the check that whether this is a linalg doing reduction > > as a helper function. I haven't put it in some linalg file yet because > > currently it's too rigid and overfitting to a specific case. I plan to > > extend it and then later maybe we can move it to some linalg file. > Let's sync up over video for this, this is a longer discussion and touches > phase ordering issues, local patterns etc. > If you want some of the deeper details please look at the rationale doc: > https://mlir.llvm.org/docs/Dialects/Linalg/ (which will soon move to > https://mlir.llvm.org/docs/Dialects/LinalgRationale/). > > In the meantime, this is not a blocker but it would be great to sync up in > particular on the implementation on those ideas today in DRR. Awesome, thanks! I've opened https://mlir.llvm.org/docs/Dialects/Linalg/ in a tab but haven't read it through yet. ;-P Will do now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73437/new/ https://reviews.llvm.org/D73437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
nicolasvasilache accepted this revision. nicolasvasilache added a comment. This revision is now accepted and ready to land. Herald added a subscriber: Joonsoo. Thanks Lei, this looks great, glad to see you pushing on this front! Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:125 + +PatternMatchResult SingleWorkgroupReduction::matchAndRewrite( +linalg::GenericOp genericOp, ArrayRef operands, antiagainst wrote: > nicolasvasilache wrote: > > Please split this into a precondition and an apply that must succeed so we > > can use the precondition as a Constraint and the apply as a simple Rewrite. > > I have found this split to be crucial to write custom fused passes with DRR. > > I claim this is generally the way we should be writing transformations when > > we can, which is the case for Linalg ops. > I feel I don't have all the backgrounds here so I'd appreciate some > explanation as for why it's necessary to separate the match and rewrite. I > think that's the original design of patterns (that we have `match` and > `rewrite` separately) and then later we found it's too cumbersome to carry > over a large matched state between them and then came up with > `matchAndRewrite`? IIUC, every pattern is applied as a whole. I can > understand that the match/rewrite side can be potentially reusable across > patterns as components if they are exposed as helper functions, but that's > more from a library organization and code reuse's perspective; I feel you've > more reasons than that so I'd like to learn about them. :) > > Another question here is that I've matches that is generally useful to other > patterns (checking whether this is a linalg doing reduction) and matches that > is only useful to SPIR-V here (checking whether the workload can be fit in > one local workgroup). How do you handle such cases? > > For now I separated the check that whether this is a linalg doing reduction > as a helper function. I haven't put it in some linalg file yet because > currently it's too rigid and overfitting to a specific case. I plan to extend > it and then later maybe we can move it to some linalg file. Let's sync up over video for this, this is a longer discussion and touches phase ordering issues, local patterns etc. If you want some of the deeper details please look at the rationale doc: https://mlir.llvm.org/docs/Dialects/Linalg/ (which will soon move to https://mlir.llvm.org/docs/Dialects/LinalgRationale/). In the meantime, this is not a blocker but it would be great to sync up in particular on the implementation on those ideas today in DRR. Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:156 + + auto inputMap = genericOp.indexing_maps().getValue()[0].cast(); + auto outputMap = antiagainst wrote: > nicolasvasilache wrote: > > I have similar comments re utils here but it will take a bit longer to put > > things in a reasonable form so please just add a TODO that this should use > > generic Linalg utils when available. > Right. I guess here we need some thinking about how to better design the API > to make it generic and usable across different cases. I'm not so versed on > that at the current moment. So putting your name in the TODO for now. :) SG, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73437/new/ https://reviews.llvm.org/D73437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
merge_guards_bot added a comment.
{icon check-circle color=green} Unit tests: pass. 62199 tests passed, 0 failed
and 815 were skipped.
{icon check-circle color=green} clang-tidy: pass.
{icon times-circle color=red} clang-format: fail. Please format your changes
with clang-format by running `git-clang-format HEAD^` or applying this patch
.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
,
test-results.xml
//Pre-merge checks is in beta. Report issue
.
Please join beta .//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
antiagainst updated this revision to Diff 241431.
antiagainst marked 16 inline comments as done.
antiagainst added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
Files:
mlir/include/mlir/Conversion/LinalgToSPIRV/LinalgToSPIRV.h
mlir/include/mlir/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.h
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h
mlir/include/mlir/Dialect/Linalg/Utils/Utils.h
mlir/include/mlir/Dialect/SPIRV/SPIRVAtomicOps.td
mlir/include/mlir/Dialect/SPIRV/SPIRVControlFlowOps.td
mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h
mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h
mlir/lib/Conversion/CMakeLists.txt
mlir/lib/Conversion/LinalgToSPIRV/CMakeLists.txt
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.cpp
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
mlir/lib/Dialect/Linalg/Utils/Utils.cpp
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
mlir/lib/Dialect/SPIRV/TargetAndABI.cpp
mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
mlir/tools/mlir-opt/CMakeLists.txt
Index: mlir/tools/mlir-opt/CMakeLists.txt
===
--- mlir/tools/mlir-opt/CMakeLists.txt
+++ mlir/tools/mlir-opt/CMakeLists.txt
@@ -40,6 +40,7 @@
MLIRQuantOps
MLIRROCDLIR
MLIRSPIRV
+ MLIRLinalgToSPIRVTransforms
MLIRStandardToSPIRVTransforms
MLIRSPIRVTestPasses
MLIRSPIRVTransforms
Index: mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
===
--- /dev/null
+++ mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
@@ -0,0 +1,162 @@
+// RUN: mlir-opt -split-input-file -convert-linalg-to-spirv -canonicalize -verify-diagnostics %s -o - | FileCheck %s
+
+//===--===//
+// Single workgroup reduction
+//===--===//
+
+#single_workgroup_reduction_trait = {
+ args_in = 1,
+ args_out = 1,
+ iterator_types = ["reduction"],
+ indexing_maps = [
+affine_map<(i) -> (i)>,
+affine_map<(i) -> (0)>
+ ]
+}
+
+module attributes {
+ spv.target_env = {
+version = 3 : i32,
+extensions = [],
+capabilities = [1: i32, 63: i32] // Shader, GroupNonUniformArithmetic
+ }
+} {
+
+// CHECK: spv.globalVariable
+// CHECK-SAME: built_in("LocalInvocationId")
+
+// CHECK: func @single_workgroup_reduction
+// CHECK-SAME: (%[[INPUT:.+]]: !spv.ptr{{.+}}, %[[OUTPUT:.+]]: !spv.ptr{{.+}})
+
+// CHECK:%[[ZERO:.+]] = spv.constant 0 : i32
+// CHECK:%[[ID:.+]] = spv.Load "Input" %{{.+}} : vector<3xi32>
+// CHECK:%[[X:.+]] = spv.CompositeExtract %[[ID]][0 : i32]
+
+// CHECK:%[[INPTR:.+]] = spv.AccessChain %[[INPUT]][%[[ZERO]], %[[X]]]
+// CHECK:%[[VAL:.+]] = spv.Load "StorageBuffer" %[[INPTR]] : i32
+// CHECK:%[[ADD:.+]] = spv.GroupNonUniformIAdd "Subgroup" "Reduce" %[[VAL]] : i32
+
+// CHECK:%[[OUTPTR:.+]] = spv.AccessChain %[[OUTPUT]][%[[ZERO]], %[[ZERO]]]
+// CHECK:%[[ELECT:.+]] = spv.GroupNonUniformElect "Subgroup" : i1
+
+// CHECK:spv.selection {
+// CHECK: spv.BranchConditional %[[ELECT]], ^bb1, ^bb2
+// CHECK:^bb1:
+// CHECK: spv.AtomicIAdd "Device" "AcquireRelease" %[[OUTPTR]], %[[ADD]]
+// CHECK: spv.Branch ^bb2
+// CHECK:^bb2:
+// CHECK: spv._merge
+// CHECK:}
+// CHECK:spv.Return
+
+func @single_workgroup_reduction(%input: memref<16xi32>, %output: memref<1xi32>) attributes {
+ spv.entry_point_abi = {local_size = dense<[16, 1, 1]>: vector<3xi32>}
+} {
+ linalg.generic #single_workgroup_reduction_trait %input, %output {
+^bb(%in: i32, %out: i32):
+ %sum = addi %in, %out : i32
+ linalg.yield %sum : i32
+ } : memref<16xi32>, memref<1xi32>
+ spv.Return
+}
+}
+
+// -
+
+// Missing shader entry point ABI
+
+#single_workgroup_reduction_trait = {
+ args_in = 1,
+ args_out = 1,
+ iterator_types = ["reduction"],
+ indexing_maps = [
+affine_map<(i) -> (i)>,
+affine_map<(i) -> (0)>
+ ]
+}
+
+module attributes {
+ spv.target_env = {
+version = 3 : i32,
+extensions = [],
+capabilities = [1: i32, 63: i32] // Shader, GroupNonUniformArithmetic
+ }
+} {
+func @single_workgroup_reduction(%input: memref<16xi32>, %output: memref<1xi32>) {
+ // expected-error @+1 {{failed to legalize operation 'linalg.generic'}}
+ linalg.generic #single_workgroup_reduction_trait %input, %output {
+^bb(%in: i32, %out: i32):
+ %sum = addi %in, %out : i32
+ linalg.yield %sum : i32
+ } : memref<16xi32>, memref<1xi32>
+ return
+}
+}
+
+// -
+
+// Mismatch between shader entry
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
antiagainst added inline comments.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:182
+ // Perform the group reduction operation.
+ Value groupOperation = rewriter.create(
+ loc, originalInputType.getElementType(), spirv::Scope::Subgroup,
mravishankar wrote:
> mravishankar wrote:
> > antiagainst wrote:
> > > mravishankar wrote:
> > > > This is hard-wiring to IAdd. We probably need to structure this
> > > > differently. We need to have a pattern to lower the linalg.generic with
> > > > reduction iterator into the kernel generated here, and then lower the
> > > > operations within the region separately.
> > > It was intentional to only support IAdd. I've re-structured this a bit so
> > > it's extensible to other binary op kinds.
> > The way I see this structured is
> >
> > 1) You check the "structure" of the linalg.generic op to see if it is a 1D
> > reduction. You assume that the body of the generic op represents a binary
> > operation that can be used for the reduction.
> > 2) You write a separate pattern that converts the operations of the body
> > itself into the spirv::GroupNonUniform*Op.
> >
> > The dialect conversion will first convert the generic op, and then it will
> > attempt to convert the body of the op. The way this is strucutred, it can
> > only handle straight-forward binary operations. THere could be more
> > complicated operations in the region of a generic op that implements the
> > reduction, which would be hard to incorporate in this approach.
> >
> > With your updated changes, it is easy to extend to other ops, but I think a
> > more general solution is needed where we are not constrained in handling
> > just simple reductions. It might need some more careful design though,
> > which I dont have a full picture of right now. So for now this OK, but
> > please leave a TODO to say something like "handle more general reductions".
> Thought about this a little bit more. It will be a more sane strategy to not
> handle cases where the region is "inlined" into the generic op, but rather
> the region is specified as a function using the `fun` attribute of the
> `linalg.generic` op. Then we can split the lowering of the generic op and the
> lowering of the function that does the reduction separately.
> This too not needed for this CL. Should revisit this
Yeah, right now linalg.generic is an integrated entity; I'm not sure how we can
easily convert part of it (the "structure") and then the inlined scalar region.
I guess it's fine for now to overfit a bit until later we see concrete cases
that we want to support, which will give us a more tangible problem to solve.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:27
+/// types.
+static bool areAllValuesMemref(Operation *op) {
+ auto isOfMemrefType = [](Value val) {
nicolasvasilache wrote:
> Please use `LinalgOp::hasBufferSemantics` instead of rolling your own
Oh, didn't know we have that. Thanks!
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:80
+ auto = op.region();
+ if (region.empty() || !has_single_element(region.getBlocks()))
+return BinaryOpKind::Unknown;
rriddle wrote:
> You should be able to remove the .empty() call as well as the .getBlocks()
Nice, thanks!
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:125
+
+PatternMatchResult SingleWorkgroupReduction::matchAndRewrite(
+linalg::GenericOp genericOp, ArrayRef operands,
nicolasvasilache wrote:
> Please split this into a precondition and an apply that must succeed so we
> can use the precondition as a Constraint and the apply as a simple Rewrite.
> I have found this split to be crucial to write custom fused passes with DRR.
> I claim this is generally the way we should be writing transformations when
> we can, which is the case for Linalg ops.
I feel I don't have all the backgrounds here so I'd appreciate some explanation
as for why it's necessary to separate the match and rewrite. I think that's the
original design of patterns (that we have `match` and `rewrite` separately) and
then later we found it's too cumbersome to carry over a large matched state
between them and then came up with `matchAndRewrite`? IIUC, every pattern is
applied as a whole. I can understand that the match/rewrite side can be
potentially reusable across patterns as components if they are exposed as
helper functions, but that's more from a library organization and code reuse's
perspective; I feel you've more reasons than that so I'd like to learn about
them. :)
Another question here is that I've matches that is generally useful to other
patterns (checking whether this is a linalg doing reduction) and matches that
is only useful to SPIR-V here (checking whether the workload can be fit in one
local workgroup). How do you handle such cases?
For now I
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
mravishankar accepted this revision. mravishankar marked 2 inline comments as done. mravishankar added inline comments. Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:73 + + using mlir::matchers::m_Val; + auto a = m_Val(block.getArgument(0)); mravishankar wrote: > antiagainst wrote: > > mravishankar wrote: > > > I feel like this is not required. If the linalg.generic operation says > > > this is a reduction, we should not be checking the region to verify that > > > it is. linalg as a contract is guaranteeing this is a reduction. > > I'm not sure I get your idea correctly; but this is checking we are doing > > the expected kind of reduction (`BinaryOp`). > That is what I am not sure we need to do. You have already checked the > generic op is a 1D loop with reduction iterator type. So the body of the > generic op is assumed to be a reduction right, i.e. a "binaryOp". Here you > are checking whether its an operation which takes two operands, but not > necessarily a binary operation that can be used for reduction. In some sense > you dont need to check that since the generic op description already told you > to assume that the region of the op represents a binary op that can be used > for reduction (this is based on my understanding of "reduction" loops in > linalg, but I need to double check). > Ok, looked into this a little bit more. This is indeed required as of now. Ideally we dont need to do this, but that is orthogonal. No issues here. Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:182 + // Perform the group reduction operation. + Value groupOperation = rewriter.create( + loc, originalInputType.getElementType(), spirv::Scope::Subgroup, mravishankar wrote: > antiagainst wrote: > > mravishankar wrote: > > > This is hard-wiring to IAdd. We probably need to structure this > > > differently. We need to have a pattern to lower the linalg.generic with > > > reduction iterator into the kernel generated here, and then lower the > > > operations within the region separately. > > It was intentional to only support IAdd. I've re-structured this a bit so > > it's extensible to other binary op kinds. > The way I see this structured is > > 1) You check the "structure" of the linalg.generic op to see if it is a 1D > reduction. You assume that the body of the generic op represents a binary > operation that can be used for the reduction. > 2) You write a separate pattern that converts the operations of the body > itself into the spirv::GroupNonUniform*Op. > > The dialect conversion will first convert the generic op, and then it will > attempt to convert the body of the op. The way this is strucutred, it can > only handle straight-forward binary operations. THere could be more > complicated operations in the region of a generic op that implements the > reduction, which would be hard to incorporate in this approach. > > With your updated changes, it is easy to extend to other ops, but I think a > more general solution is needed where we are not constrained in handling just > simple reductions. It might need some more careful design though, which I > dont have a full picture of right now. So for now this OK, but please leave a > TODO to say something like "handle more general reductions". Thought about this a little bit more. It will be a more sane strategy to not handle cases where the region is "inlined" into the generic op, but rather the region is specified as a function using the `fun` attribute of the `linalg.generic` op. Then we can split the lowering of the generic op and the lowering of the function that does the reduction separately. This too not needed for this CL. Should revisit this Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73437/new/ https://reviews.llvm.org/D73437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
mravishankar marked an inline comment as done. mravishankar added inline comments. Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:73 + + using mlir::matchers::m_Val; + auto a = m_Val(block.getArgument(0)); antiagainst wrote: > mravishankar wrote: > > I feel like this is not required. If the linalg.generic operation says this > > is a reduction, we should not be checking the region to verify that it is. > > linalg as a contract is guaranteeing this is a reduction. > I'm not sure I get your idea correctly; but this is checking we are doing the > expected kind of reduction (`BinaryOp`). That is what I am not sure we need to do. You have already checked the generic op is a 1D loop with reduction iterator type. So the body of the generic op is assumed to be a reduction right, i.e. a "binaryOp". Here you are checking whether its an operation which takes two operands, but not necessarily a binary operation that can be used for reduction. In some sense you dont need to check that since the generic op description already told you to assume that the region of the op represents a binary op that can be used for reduction (this is based on my understanding of "reduction" loops in linalg, but I need to double check). Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:182 + // Perform the group reduction operation. + Value groupOperation = rewriter.create( + loc, originalInputType.getElementType(), spirv::Scope::Subgroup, antiagainst wrote: > mravishankar wrote: > > This is hard-wiring to IAdd. We probably need to structure this > > differently. We need to have a pattern to lower the linalg.generic with > > reduction iterator into the kernel generated here, and then lower the > > operations within the region separately. > It was intentional to only support IAdd. I've re-structured this a bit so > it's extensible to other binary op kinds. The way I see this structured is 1) You check the "structure" of the linalg.generic op to see if it is a 1D reduction. You assume that the body of the generic op represents a binary operation that can be used for the reduction. 2) You write a separate pattern that converts the operations of the body itself into the spirv::GroupNonUniform*Op. The dialect conversion will first convert the generic op, and then it will attempt to convert the body of the op. The way this is strucutred, it can only handle straight-forward binary operations. THere could be more complicated operations in the region of a generic op that implements the reduction, which would be hard to incorporate in this approach. With your updated changes, it is easy to extend to other ops, but I think a more general solution is needed where we are not constrained in handling just simple reductions. It might need some more careful design though, which I dont have a full picture of right now. So for now this OK, but please leave a TODO to say something like "handle more general reductions". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73437/new/ https://reviews.llvm.org/D73437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
nicolasvasilache requested changes to this revision.
nicolasvasilache added inline comments.
This revision now requires changes to proceed.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:27
+/// types.
+static bool areAllValuesMemref(Operation *op) {
+ auto isOfMemrefType = [](Value val) {
Please use `LinalgOp::hasBufferSemantics` instead of rolling your own
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:37
+/// Returns true if the given Linalg `iterators` is one reduction.
+static bool isLinalgSingleReductionIterator(ArrayAttr iterators) {
+ if (iterators.getValue().size() != 1)
Can we create a `singleReductionIterator()` somewhere in the existing utils and
directly equality compare the `ArrayAttr iterators` against that?
I anticipate this type of things is soon going to become more and more useful,
including from Tablegen.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:78
+/// ```
+static BinaryOpKind getScalarBinaryOpKind(linalg::GenericOp op) {
+ auto = op.region();
Same thing here, we should move these to utils on the Linalg side and make
reusable as things are soon going to blow up out of control otherwise.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:125
+
+PatternMatchResult SingleWorkgroupReduction::matchAndRewrite(
+linalg::GenericOp genericOp, ArrayRef operands,
Please split this into a precondition and an apply that must succeed so we can
use the precondition as a Constraint and the apply as a simple Rewrite.
I have found this split to be crucial to write custom fused passes with DRR.
I claim this is generally the way we should be writing transformations when we
can, which is the case for Linalg ops.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:156
+
+ auto inputMap =
genericOp.indexing_maps().getValue()[0].cast();
+ auto outputMap =
I have similar comments re utils here but it will take a bit longer to put
things in a reasonable form so please just add a TODO that this should use
generic Linalg utils when available.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:260
+
+void mlir::populateLinalgToSPIRVPatterns(MLIRContext *context,
+ SPIRVTypeConverter ,
Thanks for doing this as a pattern, it will compose nicely thanks to all of
@rriddle's great work!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
rriddle added inline comments.
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVControlFlowOps.td:463
+Location loc, Value condition,
+llvm::function_ref thenBody,
+OpBuilder *builder);
Drop the llvm::.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:32
+
+ return llvm::all_of(op->getOperands(), isOfMemrefType) &&
+ llvm::all_of(op->getResults(), isOfMemrefType);
nit: You can use getOperandTypes() && getResultTypes() respectively.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:38
+static bool isLinalgSingleReductionIterator(ArrayAttr iterators) {
+ if (iterators.getValue().size() != 1)
+return false;
Can you use size() directly on the ArrayAttr?
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:66
+};
+}
+
nit: Missing end namespace comment.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:80
+ auto = op.region();
+ if (region.empty() || !has_single_element(region.getBlocks()))
+return BinaryOpKind::Unknown;
You should be able to remove the .empty() call as well as the .getBlocks()
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
merge_guards_bot added a comment.
{icon check-circle color=green} Unit tests: pass. 62199 tests passed, 0 failed
and 815 were skipped.
{icon check-circle color=green} clang-tidy: pass.
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
,
test-results.xml
//Pre-merge checks is in beta. Report issue
.
Please join beta .//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
merge_guards_bot added a comment.
{icon question-circle color=gray} Unit tests: unknown.
{icon question-circle color=gray} clang-tidy: unknown.
{icon question-circle color=gray} clang-format: unknown.
Build artifacts
:
diff.json
,
console-log.txt
//Pre-merge checks is in beta. Report issue
.
Please join beta .//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
merge_guards_bot added a comment.
{icon check-circle color=green} Unit tests: pass. 62199 tests passed, 0 failed
and 815 were skipped.
{icon check-circle color=green} clang-tidy: pass.
{icon times-circle color=red} clang-format: fail. Please format your changes
with clang-format by running `git-clang-format HEAD^` or applying this patch
.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
,
test-results.xml
//Pre-merge checks is in beta. Report issue
.
Please join beta .//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
antiagainst added a comment. Messed up the revision history with Arc... Please show the diff between "Diff 1" and "Diff 4" to check the modifications. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73437/new/ https://reviews.llvm.org/D73437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
antiagainst updated this revision to Diff 240861.
antiagainst added a comment.
Clean up unrelated commits again
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
Files:
mlir/include/mlir/Conversion/LinalgToSPIRV/LinalgToSPIRV.h
mlir/include/mlir/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.h
mlir/include/mlir/Dialect/SPIRV/SPIRVAtomicOps.td
mlir/include/mlir/Dialect/SPIRV/SPIRVControlFlowOps.td
mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h
mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h
mlir/lib/Conversion/CMakeLists.txt
mlir/lib/Conversion/LinalgToSPIRV/CMakeLists.txt
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.cpp
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
mlir/lib/Dialect/SPIRV/TargetAndABI.cpp
mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
mlir/tools/mlir-opt/CMakeLists.txt
Index: mlir/tools/mlir-opt/CMakeLists.txt
===
--- mlir/tools/mlir-opt/CMakeLists.txt
+++ mlir/tools/mlir-opt/CMakeLists.txt
@@ -40,6 +40,7 @@
MLIRQuantOps
MLIRROCDLIR
MLIRSPIRV
+ MLIRLinalgToSPIRVTransforms
MLIRStandardToSPIRVTransforms
MLIRSPIRVTestPasses
MLIRSPIRVTransforms
Index: mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
===
--- /dev/null
+++ mlir/test/Conversion/LinalgToSPIRV/linalg-to-spirv.mlir
@@ -0,0 +1,162 @@
+// RUN: mlir-opt -split-input-file -convert-linalg-to-spirv -canonicalize -verify-diagnostics %s -o - | FileCheck %s
+
+//===--===//
+// Single workgroup reduction
+//===--===//
+
+#single_workgroup_reduction_trait = {
+ args_in = 1,
+ args_out = 1,
+ iterator_types = ["reduction"],
+ indexing_maps = [
+affine_map<(i) -> (i)>,
+affine_map<(i) -> (0)>
+ ]
+}
+
+module attributes {
+ spv.target_env = {
+version = 3 : i32,
+extensions = [],
+capabilities = [1: i32, 63: i32] // Shader, GroupNonUniformArithmetic
+ }
+} {
+
+// CHECK: spv.globalVariable
+// CHECK-SAME: built_in("LocalInvocationId")
+
+// CHECK: func @single_workgroup_reduction
+// CHECK-SAME: (%[[INPUT:.+]]: !spv.ptr{{.+}}, %[[OUTPUT:.+]]: !spv.ptr{{.+}})
+
+// CHECK:%[[ZERO:.+]] = spv.constant 0 : i32
+// CHECK:%[[ID:.+]] = spv.Load "Input" %{{.+}} : vector<3xi32>
+// CHECK:%[[X:.+]] = spv.CompositeExtract %[[ID]][0 : i32]
+
+// CHECK:%[[INPTR:.+]] = spv.AccessChain %[[INPUT]][%[[ZERO]], %[[X]]]
+// CHECK:%[[VAL:.+]] = spv.Load "StorageBuffer" %[[INPTR]] : i32
+// CHECK:%[[ADD:.+]] = spv.GroupNonUniformIAdd "Subgroup" "Reduce" %[[VAL]] : i32
+
+// CHECK:%[[OUTPTR:.+]] = spv.AccessChain %[[OUTPUT]][%[[ZERO]], %[[ZERO]]]
+// CHECK:%[[ELECT:.+]] = spv.GroupNonUniformElect "Subgroup" : i1
+
+// CHECK:spv.selection {
+// CHECK: spv.BranchConditional %[[ELECT]], ^bb1, ^bb2
+// CHECK:^bb1:
+// CHECK: spv.AtomicIAdd "Device" "AcquireRelease" %[[OUTPTR]], %[[ADD]]
+// CHECK: spv.Branch ^bb2
+// CHECK:^bb2:
+// CHECK: spv._merge
+// CHECK:}
+// CHECK:spv.Return
+
+func @single_workgroup_reduction(%input: memref<16xi32>, %output: memref<1xi32>) attributes {
+ spv.entry_point_abi = {local_size = dense<[16, 1, 1]>: vector<3xi32>}
+} {
+ linalg.generic #single_workgroup_reduction_trait %input, %output {
+^bb(%in: i32, %out: i32):
+ %sum = addi %in, %out : i32
+ linalg.yield %sum : i32
+ } : memref<16xi32>, memref<1xi32>
+ spv.Return
+}
+}
+
+// -
+
+// Missing shader entry point ABI
+
+#single_workgroup_reduction_trait = {
+ args_in = 1,
+ args_out = 1,
+ iterator_types = ["reduction"],
+ indexing_maps = [
+affine_map<(i) -> (i)>,
+affine_map<(i) -> (0)>
+ ]
+}
+
+module attributes {
+ spv.target_env = {
+version = 3 : i32,
+extensions = [],
+capabilities = [1: i32, 63: i32] // Shader, GroupNonUniformArithmetic
+ }
+} {
+func @single_workgroup_reduction(%input: memref<16xi32>, %output: memref<1xi32>) {
+ // expected-error @+1 {{failed to legalize operation 'linalg.generic'}}
+ linalg.generic #single_workgroup_reduction_trait %input, %output {
+^bb(%in: i32, %out: i32):
+ %sum = addi %in, %out : i32
+ linalg.yield %sum : i32
+ } : memref<16xi32>, memref<1xi32>
+ return
+}
+}
+
+// -
+
+// Mismatch between shader entry point ABI and input memref shape
+
+#single_workgroup_reduction_trait = {
+ args_in = 1,
+ args_out = 1,
+ iterator_types = ["reduction"],
+ indexing_maps = [
+affine_map<(i) -> (i)>,
+affine_map<(i) -> (0)>
+ ]
+}
+
+module
[PATCH] D73437: [mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
antiagainst marked 2 inline comments as done.
antiagainst added inline comments.
Herald added a reviewer: mclow.lists.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:27
+/// types.
+static inline bool areAllValuesMemref(Operation *op) {
+ auto isOfMemrefType = [](Value val) {
rriddle wrote:
> Why the inline keyword?
Added initially because this function was trivial enough. I guess right now
it's better to let the compiler decide.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:60
+ auto = op.region();
+ if (region.empty() || region.getBlocks().size() != 1)
+return false;
rriddle wrote:
> Don't use size, it is O(N). Use 'has_single_element' instead.
Oh, good to know! Thanks!
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:70
+ auto = block.getOperations();
+ if (ops.size() != 2)
+return false;
rriddle wrote:
> Same here, size() is O(N): `!has_single_element(block.without_terminator())`
Nice, thanks!
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:73
+
+ using mlir::matchers::m_Val;
+ auto a = m_Val(block.getArgument(0));
mravishankar wrote:
> I feel like this is not required. If the linalg.generic operation says this
> is a reduction, we should not be checking the region to verify that it is.
> linalg as a contract is guaranteeing this is a reduction.
I'm not sure I get your idea correctly; but this is checking we are doing the
expected kind of reduction (`BinaryOp`).
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:103
+linalg::GenericOp genericOp, ArrayRef operands,
+ConversionPatternRewriter ) const {
+ Operation *op = genericOp.getOperation();
rriddle wrote:
> This function is huge, is there any way to split it up a bit?
Good point. Split out two helper functions.
Comment at: mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp:182
+ // Perform the group reduction operation.
+ Value groupOperation = rewriter.create(
+ loc, originalInputType.getElementType(), spirv::Scope::Subgroup,
mravishankar wrote:
> This is hard-wiring to IAdd. We probably need to structure this differently.
> We need to have a pattern to lower the linalg.generic with reduction iterator
> into the kernel generated here, and then lower the operations within the
> region separately.
It was intentional to only support IAdd. I've re-structured this a bit so it's
extensible to other binary op kinds.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73437/new/
https://reviews.llvm.org/D73437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
