This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Add comprehensive bufferization support for ConstantOp (13/n)
ClosedPublic

Authored by nicolasvasilache on Jun 29 2021, 3:08 PM.

Details

Summary

ConstantOp are only supported in the ModulePass because they require a GlobalCreator object that must be constructed from a ModuleOp.
If the standlaone FunctionPass encounters a ConstantOp, bufferization fails.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Jun 29 2021, 3:08 PM

Add invalid test.

silvas accepted this revision.Jun 29 2021, 3:25 PM

Can you just delete the function pass and have the module pass be the only registered pass?

This revision is now accepted and ready to land.Jun 29 2021, 3:25 PM
rriddle added inline comments.Jun 29 2021, 3:28 PM
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
2187–2196
bondhugula added inline comments.
mlir/test/Dialect/Linalg/comprehensive-func-bufferize-invalid.mlir
31–32

But for constants defined inside of functions, why should one fail bufferization and kill parallelism (using module pass instead of function pass)? Is the pass also transparently promoting the constants to global - something not immediately expected/implied from bufferization? And in any case, if this is what is being done, is there a motivation to even support the FunctionPass?

I don't find that there is a clear unambiguous way of moving forward here, only somewhat unsatisfactory options.

Forcing everything to be a ModulePass is premature atm: others have expressed a wish for generalization to "more generic ops with regions" than just func, this is still unclear.

The parallelism granularity decision and implications is a particular tradeoff in performance vs simplicity/what pass writers have to care about vs how passes must be written.
I greatly appreciate the "parallelism for free" aspect that comes with FuncOp granularity so I don't imagine this is going to change (ever?) to give users more control as it would be a pretty big footgun.
I wonder however whether ModuleOp could gain a "synchronized region" for things that are not functions?
@herhut also mentioned wishes for transactional updates du ModuleOp.

The "transparently promoting the constants to global" part is debatable.
Atm, tensor constant bufferization is a specific independent ModuleOp pass.
Going to memref.global easily turns this into readonly memory and is pretty clean; the alternatives I see are either to unroll constants or plop them into a vector + vector.store, both of which have large drawbacks.

Another alternative that I'm leaning towards is to allow memref.global to be constructible within a FuncOp but illegal to lower to LLVM.
During conversion to LLVM, legalization would consist in hoisting to the first enclosing ModuleOp.

silvas added a comment.EditedJun 30 2021, 9:33 AM

I would personally like to see this as a nice clean module pass, and then make targeted changes informed by concrete use cases (actual need for more parallelism, need to be applied locally within a particular kind of enclosing op, etc.). Really for more advanced use cases this is going to be accessed via a direct C++ API, so the pass structure we choose here should be clean and simple and have nice invariants that make it easy to reason about extracting a targeted C++ API for new use cases. For example, the C++ API could take a std::function<Value(ConstantOp op)> for creating memref values from constants, which for the module pass would use a GlobalCreator, but for other users would be implemented otherwise. I don't want to promote uses of ComprehensiveBufferize with this weird non-support for constants.

I don't think we want to allow memref.global inside a function. That just seems like a huge layering violation (e.g. it is a Symbol but a FuncOp is not a SymbolTable).

nicolasvasilache retitled this revision from [mlir][Linalg] Add comprehensive bufferization support for ConstantOp (12/n) to [mlir][Linalg] Add comprehensive bufferization support for ConstantOp (13/n).Jun 30 2021, 2:14 PM

After considering various options, @silvas was the one that made the most sense.

Rebase on dropped func-bufferize pass.

This revision was landed with ongoing or failed builds.Jul 1 2021, 4:51 AM
This revision was automatically updated to reflect the committed changes.
nicolasvasilache marked an inline comment as done.