This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Adding standard pipeline for tests.
ClosedPublic

Authored by wrengr on Jan 21 2022, 12:32 PM.

Diff Detail

Event Timeline

wrengr created this revision.Jan 21 2022, 12:32 PM
wrengr requested review of this revision.Jan 21 2022, 12:32 PM

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?

wrengr added inline comments.Jan 21 2022, 1:39 PM
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?

mehdi_amini added inline comments.Jan 21 2022, 2:08 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorPasses.cpp
142

You're right, this is terrible...
The pipeline registration is very old, and has no in-tree user. This is also somehow obsolete by the dynamic pipeline that we have now, maybe we should remove this pipeline registration mechanism entirely.

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

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.

aartbik added inline comments.Jan 21 2022, 3:20 PM
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.

aartbik added inline comments.Jan 21 2022, 3:21 PM
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.

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

I think we have a much deeper problem here actually, which is reflected in the BUILD file.

Yeah, I noticed that. Any way to avoid that would be really great ;-)

mehdi_amini added inline comments.Jan 21 2022, 10:51 PM
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir
10

, but they sometimes are simply a convenient way to provide some "mechanisms" before a "policy" can be determined

Feel like I am missing something: are they more convenient than an enum?
Unless you're doing arithmetic on these numbers, I would think that using None, DenseInnerLoop, and AnyStorageInnerLoop would just be as convenient to use?

aartbik added inline comments.Jan 22 2022, 1:32 PM
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_mttkrp.mlir
10

Feel like I am missing something: are they more convenient than an enum?

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.

wrengr marked 4 inline comments as done.Jan 24 2022, 1:12 PM
wrengr added inline comments.
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.

wrengr updated this revision to Diff 402729.Jan 24 2022, 6:23 PM

Factoring out the pipeline into a separate library / build-target.

wrengr marked an inline comment as done.Jan 24 2022, 6:27 PM

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

mehdi_amini accepted this revision.Jan 24 2022, 6:39 PM

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!

This revision is now accepted and ready to land.Jan 24 2022, 6:39 PM

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.

aartbik added inline comments.Jan 25 2022, 11:39 AM
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 ;-)

wrengr updated this revision to Diff 403047.Jan 25 2022, 2:58 PM
wrengr marked 3 inline comments as done.

Fixing nits

aartbik added inline comments.Jan 25 2022, 4:56 PM
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 ....

wrengr updated this revision to Diff 403094.Jan 25 2022, 5:55 PM
wrengr marked 2 inline comments as done.

fixing CMakeLists.txt

wrengr updated this revision to Diff 403442.Jan 26 2022, 4:45 PM

Renaming sparse-tensor-standard-pipeline to sparse-compiler

wrengr marked an inline comment as done.Jan 26 2022, 4:45 PM
aartbik accepted this revision.Jan 26 2022, 6:36 PM
This revision was landed with ongoing or failed builds.Jan 28 2022, 3:11 PM
This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.Jan 30 2022, 10:12 PM
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).

wrengr added inline comments.Jan 31 2022, 2:53 PM
mlir/lib/Dialect/SparseTensor/Pipelines/SparseTensorPipelines.cpp
29

This should be fixed by D118658