[PATCH] D93591: [MLIR][SPIRV] Add (de-)serialization support for SpecConstantOpeation.
antiagainst accepted this revision. antiagainst added a comment. Awesome, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93591/new/ https://reviews.llvm.org/D93591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93591: [MLIR][SPIRV] Add (de-)serialization support for SpecConstantOpeation.
antiagainst requested changes to this revision.
antiagainst added a comment.
This revision now requires changes to proceed.
Awesome! I just have a few nits left now. Marked as blocking because we need to
avoid change Clang code.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:623
{"referencesProvider", true},
-{"astProvider", true},
+{"astProvider", true}, // clangd extension
{"executeCommandProvider",
Accidental change or rebase leftover?
Anyway we need to revert the change here. :)
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:557
+ // mapping.
+ DenseMap>>
+ specConstOperationMap;
Just define a `struct` instead of using `std::tuple`? It will make the code
more readable. Also we should use `SmallVector` instead of `ArrayRef` here. I
guess ArrayRef is okay because we have access to the underlying SPIR-V blob
right now; but it can be a potential hazard given ArrayRef does not owns the
data.
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:1811
+ // that there is no need to update this fake ID since we only need to
+ // reference the creaed Value for the enclosed op from the spv::YieldOp
+ // created later in this method (both of which are the only values in their
s/creaed/created/
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:1816
+ // previous Value assigned to it isn't visible in the current scope anyway.
+ DenseMap newValueMap;
+ std::swap(valueMap, newValueMap);
What about using `llvm::SaveAndRestore` to avoid manually swap?
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:1791
+
+ // Since the enclosed op is emitted in the current block, split it in a
+ // separate new block.
ergawy wrote:
> antiagainst wrote:
> > What about first creating the spec op, set the insertion point to its body
> > (with proper guard), and then process the transient instruction? This can
> > avoid us from doing the block manipulation right? Following the above
> > comment, this is part of swapping the "context".
> If we do this, since the transient instruction has operands that are
> constants, global vars, or spec constants, then the instructions to
> materialize these references will be emitted in the spec op's region. This is
> the main reason I think we need such manipulation here; we first need to
> materialize all references, if any, in the parent region and then isolate the
> spec op's enclosed op.
>
> Let me know if what you are saying is going over my head :D.
You are right. Sorry I left this comment earlier than the comment about
serializing into `typesGlobalValues`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93591/new/
https://reviews.llvm.org/D93591
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83088: Introduce CfgTraits abstraction
antiagainst added inline comments.
Comment at: mlir/include/mlir/IR/Dominance.h:49
+template <>
+struct llvm::CfgTraitsFor {
+ using CfgTraits = mlir::CfgTraits;
rriddle wrote:
> This seems to have broken the GCC5 build:
> https://buildkite.com/mlir/mlir-core/builds/8739#7a957564-9850-487c-a814-c6818890bd14
>
> ```
> /mlir/include/mlir/IR/Dominance.h:49:14: error: specialization of
> 'template struct llvm::CfgTraitsFor' in different
> namespace [-fpermissive]
> struct llvm::CfgTraitsFor {
> ^
> In file included from mlir/include/mlir/IR/Dominance.h:13:0,
> from mlir/lib/IR/Verifier.cpp:30:
> llvm/include/llvm/Support/CfgTraits.h:294:44: error: from definition of
> 'template struct llvm::CfgTraitsFor' [-fpermissive]
> template struct CfgTraitsFor;
> ```
Pushed
https://github.com/llvm/llvm-project/commit/f2a06875b604c00cbe96e54363f4f5d28935d610
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83088/new/
https://reviews.llvm.org/D83088
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84780: Setting the /bigobj option globally for Windows debug build. https://bugs.llvm.org/show_bug.cgi?id=46733
antiagainst accepted this revision. antiagainst added a comment. This revision is now accepted and ready to land. LGTM for SPIR-V side. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84780/new/ https://reviews.llvm.org/D84780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D79830: Add support of __builtin_expect_with_probability
antiagainst added inline comments.
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:62
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+ } else {
FYI: this breaks GCC5:
https://buildkite.com/mlir/mlir-core/builds/5841#4a7c245b-9841-44aa-a82a-97686b04b672
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79830/new/
https://reviews.llvm.org/D79830
___
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
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
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
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
[PATCH] D25669: [Driver] Simplify ToolChain::GetCXXStdlibType (NFC)
zlei added a comment. LGTM https://reviews.llvm.org/D25669 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
