Using callbacks for allocation/deallocation allows users to override
the default.
Also add an option to comprehensive bufferization pass to use alloca
instead of allocs. Note that this option is just for testing. The
option to use alloca does not work well with the option to allow for
returning memrefs.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/Linalg/Passes.h | ||
---|---|---|
67 ↗ | (On Diff #381046) | I have very strong concerns about C++ injection in passes I pinged here just yesterday: https://reviews.llvm.org/D110678 on this topic. |
@mehdi_amini I see your concerns with reproducers, but what do you suggest here. We want to be able to use an allocator that make sense for the use case. For example, there might be constraints of not being able to do heap allocations. For CUDA you might want to do allocations on scratch pad memory. All this is easier to reason about while generating the allocation, rather than doing it as a later pass.
Maybe a way forward here is to not have a way to override the allocator used at the pass granularity, but rather have the utility function mlir::linalg::bufferize take call backs for allocator and deallocator. That way any pass that calls this utility function will be explicit about its allocation functions, and that should not affect the reproducer?
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp | ||
---|---|---|
1637–1641 | I'm wondering if all these bufferize functions could become member functions of a new class (or LinalgComprehensiveModuleBufferize) which has builder, aliasInfo etc. as fields, so we don't have to pass this stuff around. Anyway, not needed for this revision. | |
2363–2369 | I would probably have separate allocationFns: One for alloc and one for alloca. The implementation of these could then call some function that has the common functionality. | |
2932–2944 | If possible, I would rewrite this to something like: if (!allocationFns) { if (useAlloca) allocationFns = std::make_unique<AllocationCallbacks>(&AllocaStrategy::allocate, &AllcaStrategy::deallocate); } else { allocationFns = std::make_unique<AllocationCallbacks>(&AllocStrategy::allocate, &AllocStrategy::deallocate); } Where AllocStrategy::allocate etc. are static functions. And drop the field initializers in AllocationCallbacks. Maybe even a class AllocStrategy with fully virtual functions allocate and deallocate that can be overridden by specific implementations (instead of AllocationCallbacks). (But I people don't like virtual functions here in general, so...) | |
utils/bazel/llvm-project-overlay/mlir/BUILD.bazel | ||
6300 | Maybe this also requires updating some CMakeFile? |
Rebase and address comments.
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp | ||
---|---|---|
1637–1641 | I dont have a strong opinion. I am more in favor of modular static functions with well defined interface. The class approach leads to having state across methods and weird coupling. | |
2932–2944 | I can try to clean this up. In terms of a class and inheritance, again the issue for me is the state that you keep. Call backs are restrictive in terms of what they use (well defined by the interface). So it forces you to keep things hermitic. Classes can lead to fairly complex state being passed around. Callbacks are in general how things are being done in Linalg in other places. | |
utils/bazel/llvm-project-overlay/mlir/BUILD.bazel | ||
6300 | The CMake file is updated (the builds pass). The CMake file is nested with the source folder and not at the top level like bazel build file is. |
I'm wondering if all these bufferize functions could become member functions of a new class (or LinalgComprehensiveModuleBufferize) which has builder, aliasInfo etc. as fields, so we don't have to pass this stuff around.
Anyway, not needed for this revision.