Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mlir][linalg] Create a dedicated target for LinalgRelayoutInterface #128485

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

banach-space
Copy link
Contributor

Creates an interface target for LinalgRelayoutInterface. This is
primarily to reduce the dependency of Tensor on Linalg to the
required minimum. For context and rationale, see:

Note, I also took the liberty of renaming LinalgRelayoutInterface as
RelayoutOpInterface (removed Linalg, added Op).

Creates an interface target for `LinalgRelayoutInterface`. This is
primarily to reduce the dependency of `Tensor` on `Linalg` to the
required minimum. For context and rationale, see:
  * llvm#127668

Note, I also took the liberty of renaming `LinalgRelayoutInterface` as
`RelayoutOpInterface` (removed `Linalg`, added `Op`).
@llvmbot
Copy link
Member

llvmbot commented Feb 24, 2025

@llvm/pr-subscribers-mlir-tensor
@llvm/pr-subscribers-mlir-linalg

@llvm/pr-subscribers-mlir

Author: Andrzej Warzyński (banach-space)

Changes

Creates an interface target for LinalgRelayoutInterface. This is
primarily to reduce the dependency of Tensor on Linalg to the
required minimum. For context and rationale, see:

Note, I also took the liberty of renaming LinalgRelayoutInterface as
RelayoutOpInterface (removed Linalg, added Op).


Full diff: https://github.com/llvm/llvm-project/pull/128485.diff

7 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt (+17)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h (+3)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td (-10)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td (+1)
  • (added) mlir/include/mlir/Dialect/Linalg/IR/RelayoutOpInterface.h (+18)
  • (added) mlir/include/mlir/Dialect/Linalg/IR/RelayoutOpInterface.td (+25)
  • (modified) mlir/lib/Dialect/Tensor/IR/TensorOps.cpp (+1-1)
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
index efd708c5e5a11..386d2f3870011 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
@@ -25,6 +25,22 @@ function(add_linalg_ods_yaml_gen yaml_ast_file output_file)
   set(LLVM_TARGET_DEPENDS ${LLVM_TARGET_DEPENDS} PARENT_SCOPE)
 endfunction()
 
+# NOTE: `add_mlir_interface(interface)` adds `interface` as a dependency of
+# mlir-generic-headers, i.e.:
+#   * mlir-generic-headers -> interface
+# In addition, we have an existing MLIR-wide dependency of:
+#   * mlir-headers -> mlir-generic-headers.
+# Now, observe that:
+#   1. The targets below define _new_ dependencies for mlir-headers.
+#   2. Before the new targets are defined, `add_linalg_ods_yaml_gen` updates
+#     LLVM_TARGET_DEPENDS.
+#   3. All tablegen targets pick-up LLVM_TARGET_DEPENDS.
+# In order to avoid cyclic dependencies, we need to invoke
+# `add_mlir_interface` (and update `mlir-generic-headers`) _before_
+# LLVM_TARGET_DEPENDS is updated and new dependencies for `mlir-headers` are
+# defined + added.
+add_mlir_interface(RelayoutOpInterface)
+
 # NOTE: LLVM_TARGET_DEPENDS gets picked up by tablegen targets to add file
 # level dependencies. This is gross but CMake requires depending on both
 # targets and generated files, and it must be done when the custom target is
@@ -77,3 +93,4 @@ mlir_tablegen(LinalgInterfaces.h.inc -gen-op-interface-decls)
 mlir_tablegen(LinalgInterfaces.cpp.inc -gen-op-interface-defs)
 add_public_tablegen_target(MLIRLinalgInterfacesIncGen)
 add_dependencies(mlir-headers MLIRLinalgInterfacesIncGen)
+
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h b/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
index 6f1c243cc4396..df32cafd2d024 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
@@ -221,4 +221,7 @@ LogicalResult verifyStructuredOpInterface(Operation *op);
 /// Include the generated interface declarations.
 #include "mlir/Dialect/Linalg/IR/LinalgInterfaces.h.inc"
 
+/// Include the generated relayout interface declarations.
+#include "mlir/Dialect/Linalg/IR/RelayoutOpInterface.h.inc"
+
 #endif // MLIR_DIALECT_LINALG_IR_LINALGINTERFACES_H_
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td b/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
index 247afc141c180..dbc1ac60e0973 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
@@ -178,16 +178,6 @@ def LinalgConvolutionOpInterface : OpInterface<"ConvolutionOpInterface"> {
   ];
 }
 
-def LinalgRelayoutOpInterface : OpInterface<"RelayoutOpInterface"> {
-  let description = [{
-    A Linalg relayout-op is either linalg.pack or linalg.unpack.
-
-    While we could extend this interface with methods from Linalg_RelayoutOp,
-    this is currently not needed and left as a TODO.
-  }];
-  let cppNamespace = "::mlir::linalg";
-}
-
 def LinalgFillOpInterface : OpInterface<"FillOpInterface"> {
   let description = [{
     A fill operation is defined in general terms:
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td b/mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td
index a08a778fc25e1..1e48a5e3a20ee 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td
@@ -23,6 +23,7 @@ include "mlir/Interfaces/DestinationStyleOpInterface.td"
 include "mlir/Interfaces/SideEffectInterfaces.td"
 include "mlir/Interfaces/InferTypeOpInterface.td"
 include "mlir/Dialect/Linalg/IR/LinalgInterfaces.td"
+include "mlir/Dialect/Linalg/IR/RelayoutOpInterface.td"
 include "mlir/IR/OpAsmInterface.td"
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/RelayoutOpInterface.h b/mlir/include/mlir/Dialect/Linalg/IR/RelayoutOpInterface.h
new file mode 100644
index 0000000000000..5e3c625558508
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Linalg/IR/RelayoutOpInterface.h
@@ -0,0 +1,18 @@
+//===- RelayoutOpInterface.h - Relayout op interface ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements the operation interface for relayout ops.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_DIALECT_RELAYOUTOPINTERFACE_H_
+#define MLIR_DIALECT_RELAYOUTOPINTERFACE_H_
+
+#include "mlir/Dialect/Linalg/IR/RelayoutOpInterface.h.inc"
+
+#endif // MLIR_DIALECT_RELAYOUTOPINTERFACE_H_
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/RelayoutOpInterface.td b/mlir/include/mlir/Dialect/Linalg/IR/RelayoutOpInterface.td
new file mode 100644
index 0000000000000..2dec2fc4396f4
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Linalg/IR/RelayoutOpInterface.td
@@ -0,0 +1,25 @@
+//===- RelayoutOpInterface.td ----- Intrerface Declaration -*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LINALG_IR_RELAYOUTOPINTERFACE
+#define LINALG_IR_RELAYOUTOPINTERFACE
+
+include "mlir/Interfaces/DestinationStyleOpInterface.td"
+include "mlir/IR/OpBase.td"
+
+def LinalgRelayoutOpInterface : OpInterface<"RelayoutOpInterface"> {
+  let description = [{
+    A Linalg relayout-op is either linalg.pack or linalg.unpack.
+
+    While we could extend this interface with methods from Linalg_RelayoutOp,
+    this is currently not needed and left as a TODO.
+  }];
+  let cppNamespace = "::mlir::linalg";
+}
+
+#endif // LINALG_IR_RELAYOUTOPINTERFACE
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index fad7db48b9872..e3c8899cd44d9 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -10,7 +10,7 @@
 #include "mlir/Dialect/Arith/IR/Arith.h"
 #include "mlir/Dialect/Arith/Utils/Utils.h"
 #include "mlir/Dialect/Complex/IR/Complex.h"
-#include "mlir/Dialect/Linalg/IR/LinalgInterfaces.h"
+#include "mlir/Dialect/Linalg/IR/RelayoutOpInterface.h"
 #include "mlir/Dialect/Tensor/IR/Tensor.h"
 #include "mlir/Dialect/Utils/IndexingUtils.h"
 #include "mlir/Dialect/Utils/ReshapeOpsUtils.h"

Copy link
Member

@rengolin rengolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but please wait some time for other reviews.

Thanks for working on this, I don't think we can reduce the dependency more than this...

@chsigg
Copy link
Contributor

chsigg commented Feb 24, 2025

Looks good to me. I will fix up the bazel build after this one landed.

@banach-space
Copy link
Contributor Author

Thank you all for the discussion! If there are no fresh comments, I will land this later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants