This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Retire LinalgGeneralization
AbandonedPublic

Authored by guraypp on Aug 18 2022, 1:43 AM.

Details

Summary

This revision is in work in progress. Its aim to retire LinalgGeneralization pattern/pass and use transform.structured.generalize.

I came across two problems. The first one is related to the sparse compiler; the following tests are failing. I think transform dialect is not enabled with --sparse-compiler. For example in sparse_dot.mlir the line linalg.dot is not lowered using transform dialect.

MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_dot.mlir
MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_filter_conv2d.mlir
MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_matmul.mlir
MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_quantized_matmul.mlir
MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_sum.mlir
MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_sum_bf16.mlir
MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_sum_f16.mlir
MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_vector_ops.mlir

The second problem is more like a question. Should I remove LinalgGeneralizationPattern? Currently the tests that I edit work fine with transform dialect, but I am not sure whether I am using it in the right way.

Diff Detail

Event Timeline

guraypp created this revision.Aug 18 2022, 1:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
guraypp requested review of this revision.Aug 18 2022, 1:43 AM
aartbik requested changes to this revision.EditedAug 18 2022, 10:42 AM

I usually try to stay away from giving a -1, but I would really like to understand the rationale behind this change better.
I really don't want to run "--test-transform-dialect-interpreter" in the tests to be able to a handle a sparse linalg.dot for example.

This revision now requires changes to proceed.Aug 18 2022, 10:42 AM

I usually try to stay away from giving a -1, but I would really like to understand the rationale behind this change better.
I really don't want to run "--test-transform-dialect-interpreter" in the tests to be able to a handle a sparse linalg.dot for example.

This revision was originally created to consult you. We have an intention to retire the LinalgGeneralizationPattern and use a transfrom dialect.

Could you please elaborate why you wouldn't want to use "--test-transform-dialect-interpreter" in the sparse compiler (especially for someone who is just starting out and doesn't have a full grasp of the gist :) )

I usually try to stay away from giving a -1, but I would really like to understand the rationale behind this change better.
I really don't want to run "--test-transform-dialect-interpreter" in the tests to be able to a handle a sparse linalg.dot for example.

This revision was originally created to consult you. We have an intention to retire the LinalgGeneralizationPattern and use a transfrom dialect.

Could you please elaborate why you wouldn't want to use "--test-transform-dialect-interpreter" in the sparse compiler (especially for someone who is just starting out and doesn't have a full grasp of the gist :) )

By the way, I believe @nicolasvasilache could explain the rational behind this change better than I tried

This revision was originally created to consult you. We have an intention to retire the LinalgGeneralizationPattern and use a transfrom dialect.
Could you please elaborate why you wouldn't want to use "--test-transform-dialect-interpreter" in the sparse compiler (especially for someone who is just starting out and doesn't have a full grasp of the gist :) )

Happy to work together on this, of course!

The sparse compiler is pipeline (i.e. --sparse-compiler is a shorthand for a complete pipeline with many passes). So, at the very least, I would like to see any new pass that is needed incorporated into that pipeline, and not having to add extra passes to the command line. Second, right now, one would expect %dot = linalg.dot to just work with the sparse compiler and the given source file for the test. I am a bit unclear why the transform pattern has to be added to the source code (at the very least, I would expect a set of built-in transformation patterns that replace the named linalg op rewriting; or even when not built-in, one centralized place with such patterns, rather than adding them to each source).

@aartbik the general context is https://discourse.llvm.org/t/psa-retire-linalg-filter-based-patterns/63785

I agree we don't want to pollute the sparse tensor tests with the transform dialect.

@guraypp , we want to keep

def LinalgGeneralization : Pass<"linalg-generalize-named-ops", "func::FuncOp"> {

but we want to drop the filter-based one:

def LinalgStrategyGeneralizePass
    : Pass<"linalg-strategy-generalize-pass", "func::FuncOp"> {

The attribute-based filters and things that related to "LinalgStrategies" are what is replaced by the transform dialect.
This should all be transparent for the sparse tensor pipeline and I think for almost all the tests (unless you see something specific with an attribute being used to target transforms).

mlir/lib/Dialect/SparseTensor/Pipelines/SparseTensorPipelines.cpp
55

this should stay.

mlir/test/Dialect/Linalg/generalize-named-ops.mlir
1

This shouldn't change, only passes that use "filters" should be cleaned up.

nicolasvasilache requested changes to this revision.Aug 22 2022, 2:40 AM
guraypp abandoned this revision.Aug 22 2022, 2:53 AM