Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Sweet! LGTM overall, but I'll let Aart have another look swell.
I also have one slight concerns about a global constructor for the pipeline registration.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp | ||
---|---|---|
142 | Does this build? I would expect this to be caught as a global constructor which isn't allowed in the MLIR libraries. | |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir | ||
10 | It's not coming from you here, but using opaque integer in the options is really not ideal, I'd rather see enums! Do you mind filing a bug? |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp | ||
---|---|---|
142 | It builds and works on the Google side; still waiting to see if the buildbot likes it. If there's another/better approach I'm all ears. The documentation I could find was incomplete and I couldn't find any examples in the MLIR monorepo (except for debugging the pipeline functionality itself). The examples I could find were in tensorflow and ireee, but both were inconsistent about defining a top-level static variable vs wrapping it in a registration function. So yeah, I'm game to switch it to whatever the right thing is :) | |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir | ||
10 | Yeah, I'm not a fan of opaque numbers either, just didn't want to handle that in the same differential. Will file a bug for it. I did locate the documentation for using enums with the LLVM commandline framework, but I didn't see anything for tablegen or MLIR specifically. Do those exist, or should I just reuse the LLVM combinators directly? |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp | ||
---|---|---|
142 | You're right, this is terrible... So I think we should try to use the dynamic pipeline mechanism here as well, do you want to sync offline on this? I can help you set this up here. | |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir | ||
10 | It just reuses LLVM functionality. Here is an example: https://github.com/llvm/llvm-project/blob/705d8c49f9be82415a9cdd793cda405333fba71e/mlir/include/mlir/Transforms/Passes.td#L138-L147 |
Nice cleanup indeed! But indeed some fix is required, please work with Mehdi.
/var/lib/buildkite-agent/builds/llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp:144:5: error: declaration requires a global constructor [-Werror,-Wglobal-constructors]
registerSparseTensorPipeline("sparse-tensor-standard-pipeline", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir | ||
---|---|---|
10 | I don't think anybody should be a fan of opaque numbers, but they sometimes are simply a convenient way to provide some "mechanisms" before a "policy" can be determined (used at other places too). There is a long term intention to keep these. |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir | ||
---|---|---|
10 |
Oops, we need an edit button for this comments. This should obviously read: There is no long term intention to keep these. |
I think we have a much deeper problem here actually, which is reflected in the BUILD file.
This introduces a lot of dependencies in the SparseTensor dialect directory to a lot of components which it shouldn't depend on (Seeing LinalgTransforms is already strange here).
What we're really missing here is very fundamental to the repository, we need to make progress on https://llvm.discourse.group/t/rfc-restructuring-of-the-mlir-repo/4927
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir | ||
---|---|---|
10 |
Feel like I am missing something: are they more convenient than an enum? |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir | ||
---|---|---|
10 |
I am not trying to defend the indefensible here ;-) just tried to give some history of why these are here (we use proper enums "internally", but I at some point in the past I added a quick hack to control this through the command line with a pass "Option" and I could only find integral examples back then). Perfectly okay with making this proper enums of course, although in the longer run we will need something like Nicolas' schedule language to control the transformations in an even better way, since they may vary over loops and the switch controls it globally now. |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir | ||
---|---|---|
10 |
Yeah, there's definitely a major issue re excessive dependencies; though I'm not sure https://llvm.discourse.group/t/rfc-restructuring-of-the-mlir-repo/4927 would fix it. Ultimately the tests do depend on all these different passes, however the rest of the SparseTensor library itself does not; so the fundamental problem is how to specify the test dependencies yet without over-constraining the library itself. To fix this we really need some sort of layering of the build dependencies, so that the pipeline can be broken out to group together with the tests themselves. (Which is afaict out of the scope of RFC-4927.) We could hack it by adding another layer of shell scripts between mlir-opt and filecheck/lilith, but (a) just using a string in the shell script loses track of the dependency information (same as the tests before this differential), and (b) layers of shell scripts are a maintenance nightmare. One possible way forward would be to restructure the pass/pipeline registration stuff to be late/lazy-binding such that: we can define the pipeline alongside the rest of the library, but those pass-registration calls are only validated when linking libraries together (ideally with a link-time error if things don't match up, rather than a run-time error). Not sure of the exact details nor if it runs counter to llvm/mlir ideology, but might be worth thinking about.
I'm not entirely sure how to adjust mlir/include/mlir/Dialect/SparseTensor/Pipelines/CMakeLists.txt given that there's no tablegen file for this library, but we still want to have the pipeline show up in the generated documentation alongside the regular passes. Any pointers would be appreciated
LG, with a nit inline!
mlir/include/mlir/Dialect/SparseTensor/Pipelines/Passes.h | ||
---|---|---|
50 | You're missing the declaration for buildSparseTensorPipeline ; I think the SparseTensorPipelineOptions won't be useful without it right ? | |
mlir/lib/Dialect/SparseTensor/Pipelines/CMakeLists.txt | ||
16 | If you're unsure that you got all the dependencies, building with -DBUILD_SHARED_LIBS=ON helps! |
Yeah, really looking good Wren!
mlir/lib/Dialect/SparseTensor/Pipelines/SparseTensorPipelines.cpp | ||
---|---|---|
56–57 | I am not sure what the right phrasing is (since your English will be better than mine), but can we elaborate a bit here on that this runs all the passes required to invoke the "sparse compiler", i.e. map sparsity agnostic IR with sparse tensor type into concrete sparse code. |
mlir/lib/Dialect/SparseTensor/Pipelines/CMakeLists.txt | ||
---|---|---|
16 | +1 I always run that build config after larger build-related changes just so I don't get unpleasant surprises later ;-) |
mlir/lib/Dialect/SparseTensor/Pipelines/SparseTensorPipelines.cpp | ||
---|---|---|
56–57 | Yes, thanks! Last nit, can we call it a sparse compiler, perhaps in between quotes or some other stylistic marker. It is kind of my pet name for such a pipeline ;-) So something like "Pipeline of a `sparse compiler' that takes sparsity-agnostic .... |
mlir/lib/Dialect/SparseTensor/Pipelines/SparseTensorPipelines.cpp | ||
---|---|---|
29 | What operation is pm here expected to run on? It looks like this builder is relying on an implicitly nesting pass manager (e.g. createLinalgBufferizePass runs on a FuncOp, but createSparsificationPass runs on a ModuleOp). Pass pipelines should be explicit about the pass manager nesting, otherwise if implicit nesting isn't enabled (in many cases) the pipeline will fail to add to a pass manager (likely causing the problems in D118579). |
You're missing the declaration for buildSparseTensorPipeline ; I think the SparseTensorPipelineOptions won't be useful without it right ?