[MLIR] Remove the dependency between Tensor and Linalg

[rengolin]

As exposed by:

• https://discourse.llvm.org/t/rfc-move-tensor-pack-and-tensor-unpack-into-linalg/
• [mlir][tensor][linalg] Move Pack/Unpack Ops to Linalg #123902
• [mlir][tensor] Introduce TensorRelayoutOpInterface #125823
• [mlir][bazel] Port https://github.com/llvm/llvm-project/commit/517800e37e8d3a4ee84214bef65e227612c2a98b #127544

We need to remove the dependencies between these two dialects. The work is ongoing, we can tag the remaining PRs with this issue to link them all in one topic. Also discuss state and direction.

@jpienaar @ftynse @banach-space

[llvmbot]

@llvm/issue-subscribers-mlir

Author: Renato Golin (rengolin)

As exposed by: * https://discourse.llvm.org/t/rfc-move-tensor-pack-and-tensor-unpack-into-linalg/ * #123902 * #125823 * #127544

We need to remove the dependencies between these two dialects. The work is ongoing, we can tag the remaining PRs with this issue to link them all in one topic. Also discuss state and direction.

@jpienaar @ftynse @banach-space

[banach-space]

Thanks Renato!

Regarding LinalgRelayoutOpInterface (that I've introduced), that's currently used in Tensor to prevent FoldTensorCastProducerOp from folding tensor.cast + linalg.pack and tensor.cast + linalg.upack, see below:

llvm-project/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp

Lines 3913 to 3917 in 055872a

	// Reject PackOp/UnpackOp (i.e. RelayoutOps) - there are dedicated patterns
	// for that instead.
	if (!foldTensorCastPrecondition(op) ||
	isa<linalg::RelayoutOpInterface>(*op))
	return failure();

Note, there are dedicated patterns for folding tensor.cast with linalg.pack + linalg.unpack in Linalg, see e.g. FoldTensorCastPack.

How can we get rid of LinalgRelayoutOpInterface from Tensor?

One option would be to make FoldTensorCastPack reject non-tensor Ops, but I suspect that's going to break something for somebody. That said, perhaps dialect folders should always reject "foreign" Ops?

[rengolin]

One potential solution is to have interfaces in a shared (include-only) directory that all dialects have access to. It can still have some structure (ex. dialect directories) but that's not the same as requiring their builds and objects.

[joker-eph]

There is no need to change the directory structure for this: you can depend on a "LinalgXXXInterface" to get the generated header without having a link dependency on Linalg for example.

[banach-space]

There is no need to change the directory structure for this: you can depend on a "LinalgXXXInterface" to get the generated header without having a link dependency on Linalg for example.

Something like this: #127541?

But then: #127541 (comment).

[chsigg]

Something like this: #127541?

I'm not sure whether we want to have a target for just the RelayoutOpInterface, or all linalg interfaces. It seems that the other interfaces require the linalg dialect, so maybe they should stay as part of the linalg dialect. But really, as I said, I don't know. ;-)

If I remember correctly, we had a similar situation before where Mehdi advocated that it's fine to have a header file that mixes inline definitions for some users (without introducing a link dependency) and declarations for other users (which then require an explicit link dependency to some other target implementing those declarations).

I followed that approach in 083f80f for the bazel build. I didn't need to touch any code for this or create interface subdirectories.

The cmake equivalent would be add_mlir_interface_library, iiuc. That might require that we move the interfaces into its own Linalg/Interfaces directory (at least that's how builtin and vector dialects do it).

[joker-eph]

But then: #127541 (comment).

.h.inc should be encapsulated because they are generated and we need to track this in cmake dependencies.
However in general interfaces come with a header independent from the dialect (I mentioned example in this discussion you linked).

The cmake equivalent would be add_mlir_interface_library, iiuc. That might require that we move the interfaces into its own Linalg/Interfaces directory

Why do you need a subdirectory?
What about add_mlir_interface_library() and how other interfaces are managed like here.

[chsigg]

Thanks Mehdi. You are right, it doesn't need a subdirectory.

What's not clear to me is whether we want a separate LinalgRelayoutOpInterface.h (as you suggested here), which would be header-only. Or is the current state with just LinalgInterfaces.h sufficient from a cmake perspective (e.g., is it desirable to build the tensor dialect without building the linalg dialect, and does this work)? From bazel's perspective, the current state is sufficient but a separate LinalgRelayoutOpInterface.h would be slightly cleaner.

add_mlir_interface() is a wrapper to run tablegen. We could indeed use that for LinalgInterfaces.
add_mlir_interface_library() are two local macros which wrap add_mlir_library(). There is no need to use those because LinalgInterfaces.cpp requires the LinalgDialect (so no point in splitting it out), and LinalgRelayoutOpInterface.h would be header only.

[banach-space]

Or is the current state with just LinalgInterfaces.h sufficient from a cmake perspective (e.g., is it desirable to build the tensor dialect without building the linalg dialect, and does this work)? F

It is sufficient in the sense that "stuff works" ™ . However, there should be no dependency of Tensor on Linalg, other than the bare minimum. Having LinalgRelayoutOpInterface.h would be closer to that minimum than what we have now. Please correct me if I misunderstood/misinterpreted something.

If there are no new comments, I can prepare a PR on top of #127541. I won't be able to help with Bazel though. @chsigg , would you be OK to prepare the necessary follow-on?

Thanks!

[chsigg]

Having LinalgRelayoutOpInterface.h would be closer to that minimum than what we have now.

Agreed.

@chsigg , would you be OK to prepare the necessary follow-on?

Abolutely, happy to do the bazel side.

[banach-space]

Having LinalgRelayoutOpInterface.h would be closer to that minimum than what we have now.

Agreed.

#128485