This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Convert linalg.generic for reduction to SPIR-V ops
ClosedPublic

Authored by antiagainst on Jan 26 2020, 2:31 PM.

Details

Summary

This commit adds a pattern to lower linalg.generic for reduction
to spv.GroupNonUniform* ops. Right now this only supports integer
reduction on 1-D input memref. Shader entry point ABI is queried
to make sure that the input memref's shape matches the local
workgroup's invocation configuration. This makes sure that the
workload fits in one local workgroup so that we can leverage
SPIR-V group non-uniform operations.

linglg.generic is a structured op that preserves the right level
of information. It is easier to recognize reduction at this level
than performing analysis on loops.

This commit also exposes getElementPtr in SPIRVLowering.h given
that it's a generally useful utility function.

Diff Detail

Event Timeline

antiagainst created this revision.Jan 26 2020, 2:31 PM

Unit tests: pass. 62199 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

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 or enable it for your project.

rriddle added inline comments.Jan 26 2020, 3:00 PM
mlir/include/mlir/Conversion/LinalgToSPIRV/LinalgToSPIRV.h
4

Should be LLVM Project now.

mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
28

Why the inline keyword?

61

Don't use size, it is O(N). Use 'has_single_element' instead.

71

Same here, size() is O(N): !has_single_element(block.without_terminator())

104

This function is huge, is there any way to split it up a bit?

mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.cpp
43

Drop the trivial brace.

mlir/lib/Dialect/SPIRV/TargetAndABI.cpp
70

nit: Drop the newline here.

mravishankar requested changes to this revision.Jan 27 2020, 10:47 AM
mravishankar added inline comments.
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
37

Nit : s/interators/iterators/

74

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.

183

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.

This revision now requires changes to proceed.Jan 27 2020, 10:47 AM
antiagainst marked 14 inline comments as done.

Address comments

Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
antiagainst marked 2 inline comments as done.Jan 28 2020, 6:20 AM
antiagainst added inline comments.
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
28

Added initially because this function was trivial enough. I guess right now it's better to let the compiler decide.

61

Oh, good to know! Thanks!

71

Nice, thanks!

74

I'm not sure I get your idea correctly; but this is checking we are doing the expected kind of reduction (BinaryOp).

104

Good point. Split out two helper functions.

183

It was intentional to only support IAdd. I've re-structured this a bit so it's extensible to other binary op kinds.

antiagainst marked an inline comment as done.

Revert unrelated commits

Clean up unrelated commits again

Messed up the revision history with Arc... Please show the diff between "Diff 1" and "Diff 4" to check the modifications.

Unit tests: pass. 62199 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

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 or enable it for your project.

Unit tests: pass. 62199 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

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 or enable it for your project.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Harbormaster failed remote builds in B45124: Diff 240860!
rriddle added inline comments.Jan 28 2020, 9:23 AM
mlir/include/mlir/Dialect/SPIRV/SPIRVControlFlowOps.td
470

Drop the llvm::.

mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
33

nit: You can use getOperandTypes() && getResultTypes() respectively.

39

Can you use size() directly on the ArrayAttr?

67

nit: Missing end namespace comment.

81

You should be able to remove the .empty() call as well as the .getBlocks()

nicolasvasilache requested changes to this revision.Jan 28 2020, 9:42 AM
nicolasvasilache added inline comments.
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
28

Please use LinalgOp::hasBufferSemantics instead of rolling your own

38

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.

79

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.

126

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.

157

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.

261

Thanks for doing this as a pattern, it will compose nicely thanks to all of @rriddle's great work!

This revision now requires changes to proceed.Jan 28 2020, 9:42 AM
mravishankar marked an inline comment as done.Jan 28 2020, 4:44 PM
mravishankar added inline comments.
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
74

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).

183

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".

mravishankar accepted this revision.Jan 29 2020, 12:03 PM
mravishankar marked 2 inline comments as done.
mravishankar added inline comments.
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
74

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.

183

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

antiagainst marked 16 inline comments as done.

Address comments

mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
28

Oh, didn't know we have that. Thanks!

81

Nice, thanks!

126

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.

157

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. :)

183

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.

Unit tests: pass. 62199 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

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 or enable it for your project.

nicolasvasilache accepted this revision.Jan 31 2020, 6:06 AM

Thanks Lei, this looks great, glad to see you pushing on this front!

mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
126

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.

157

SG, thanks!

This revision is now accepted and ready to land.Jan 31 2020, 6:06 AM
antiagainst marked 4 inline comments as done.Jan 31 2020, 6:12 AM
antiagainst added inline comments.
mlir/lib/Conversion/LinalgToSPIRV/LinalgToSPIRV.cpp
126

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.

This revision was automatically updated to reflect the committed changes.
antiagainst marked an inline comment as done.