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][tensor][linalg] Move RelayoutOpInterface from linalg back to tensor #127533

Closed
wants to merge 1 commit into from

Conversation

chsigg
Copy link
Contributor

@chsigg chsigg commented Feb 17, 2025

RelayoutOpInterface is used only to identify tensor.pack/tensor.unpack ops.

This CL moves this interface back to the tensor dialect, to allow the removal of the linalg dialect dependency from the tensor dialect.

This is a partial revert of #123902.

…ensor

RelayoutOpInterface is used only to identify tensor.pack/tensor.unpack ops.

This CL moves this interface back to the tensor dialect, to allow the removal of the linalg dialect dependency from the tensor dialect.

This is a partial revert of llvm#125823.
@llvmbot llvmbot added mlir:linalg mlir mlir:tensor bazel "Peripheral" support tier build system: utils/bazel labels Feb 17, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 17, 2025

@llvm/pr-subscribers-mlir-tensor

Author: Christian Sigg (chsigg)

Changes

RelayoutOpInterface is used only to identify tensor.pack/tensor.unpack ops.

This CL moves this interface back to the tensor dialect, to allow the removal of the linalg dialect dependency from the tensor dialect.

This is a partial revert of #125823.


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

9 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Linalg/IR/Linalg.h (+1)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td (-10)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td (+2-1)
  • (modified) mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt (+6)
  • (modified) mlir/include/mlir/Dialect/Tensor/IR/Tensor.h (+6)
  • (added) mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td (+30)
  • (modified) mlir/lib/Dialect/Tensor/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Tensor/IR/TensorOps.cpp (+1-3)
  • (modified) utils/bazel/llvm-project-overlay/mlir/BUILD.bazel (+21)
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h b/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
index 57bf6305a469d..38a8d18e56b05 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
+++ b/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
@@ -10,6 +10,7 @@
 #define MLIR_DIALECT_LINALG_IR_LINALG_H
 
 #include "mlir/Bytecode/BytecodeOpInterface.h"
+#include "mlir/Dialect/Tensor/IR/Tensor.h"
 #include "mlir/Dialect/Utils/ReshapeOpsUtils.h"
 #include "mlir/Dialect/Utils/StructuredOpsUtils.h"
 #include "mlir/IR/AffineExpr.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..2b27f2c908c64 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/Tensor/IR/TensorInterfaces.td"
 include "mlir/IR/OpAsmInterface.td"
 
 //===----------------------------------------------------------------------===//
@@ -32,7 +33,7 @@ include "mlir/IR/OpAsmInterface.td"
 class Linalg_RelayoutOp<string mnemonic, list<Trait> traits = []> :
       Op<Linalg_Dialect, mnemonic, !listconcat(traits, [
         DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>,
-        DestinationStyleOpInterface, LinalgRelayoutOpInterface,
+        DestinationStyleOpInterface, TensorRelayoutOpInterface,
         ConditionallySpeculatable, NoMemoryEffect,
         DeclareOpInterfaceMethods<ReifyRankedShapedTypeOpInterface>,
         TypesMatchWith<"result type matches type of dest",
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
index cd14fe5c04561..74a05291376b3 100644
--- a/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
@@ -1,2 +1,8 @@
 add_mlir_dialect(TensorOps tensor)
 add_mlir_doc(TensorOps TensorOps Dialects/ -gen-dialect-doc)
+
+set(LLVM_TARGET_DEFINITIONS TensorInterfaces.td)
+mlir_tablegen(TensorInterfaces.h.inc -gen-op-interface-decls)
+mlir_tablegen(TensorInterfaces.cpp.inc -gen-op-interface-defs)
+add_public_tablegen_target(MLIRTensorInterfacesIncGen)
+add_dependencies(mlir-headers MLIRTensorInterfacesIncGen)
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h b/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
index eb550bb469b9f..b3ec796a72337 100644
--- a/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
+++ b/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
@@ -46,6 +46,12 @@ SmallVector<Range, 8> getOrCreateRanges(OffsetSizeAndStrideOpInterface op,
 
 #include "mlir/Dialect/Tensor/IR/TensorOpsDialect.h.inc"
 
+//===----------------------------------------------------------------------===//
+// Tensor Interfaces
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Dialect/Tensor/IR/TensorInterfaces.h.inc"
+
 //===----------------------------------------------------------------------===//
 // Tensor Dialect Operations
 //===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td b/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td
new file mode 100644
index 0000000000000..83c173b182664
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td
@@ -0,0 +1,30 @@
+//===- TensorInterfaces.td - Tensor Interfaces 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
+//
+//===----------------------------------------------------------------------===//
+//
+// This is the definition file for the structured interface sfor Tensor ops.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef TENSOR_IR_TENSORINTERFACES
+#define TENSOR_IR_TENSORINTERFACES
+
+include "mlir/Interfaces/DestinationStyleOpInterface.td"
+include "mlir/IR/OpBase.td"
+
+def TensorRelayoutOpInterface : OpInterface<"RelayoutOpInterface"> {
+  let description = [{
+    A Tensor 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::tensor";
+}
+
+#endif // TENSOR_IR_TENSORINTERFACES
+
diff --git a/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt b/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
index 5425615dac393..d9d09d6361a2f 100644
--- a/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
+++ b/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
@@ -16,6 +16,7 @@ add_mlir_dialect_library(MLIRTensorDialect
 
   DEPENDS
   MLIRTensorOpsIncGen
+  MLIRTensorInterfacesIncGen
 
   LINK_LIBS PUBLIC
   MLIRAffineDialect
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index e741144647043..eeef383ac531a 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -10,9 +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/Linalg.h"
 #include "mlir/Dialect/Tensor/IR/Tensor.h"
-#include "mlir/Dialect/Tensor/Utils/Utils.h"
 #include "mlir/Dialect/Utils/IndexingUtils.h"
 #include "mlir/Dialect/Utils/ReshapeOpsUtils.h"
 #include "mlir/Dialect/Utils/StaticValueUtils.h"
@@ -3914,7 +3912,7 @@ struct FoldTensorCastProducerOp
     // Reject PackOp/UnpackOp (i.e. RelayoutOps) - there are dedicated patterns
     // for that instead.
     if (!foldTensorCastPrecondition(op) ||
-        isa<linalg::RelayoutOpInterface>(*op))
+        isa<tensor::RelayoutOpInterface>(*op))
       return failure();
 
     SmallVector<Type> newResultTypes(op->getResultTypes());
diff --git a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
index 92aedac837197..2569847ab6efe 100644
--- a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
@@ -11140,6 +11140,7 @@ td_library(
         "include/mlir/Dialect/Linalg/IR/LinalgEnums.td",
         "include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td",
         "include/mlir/Dialect/Linalg/IR/LinalgOps.td",
+        "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td",
     ],
     includes = ["include"],
     deps = [
@@ -11150,6 +11151,7 @@ td_library(
         ":LoopLikeInterfaceTdFiles",
         ":OpBaseTdFiles",
         ":SideEffectInterfacesTdFiles",
+        ":TensorOpsTdFiles",
         ":TilingInterfaceTdFiles",
         ":ViewLikeInterfaceTdFiles",
     ],
@@ -11206,6 +11208,23 @@ gentbl_cc_library(
     deps = [":LinalgOpsTdFiles"],
 )
 
+gentbl_cc_library(
+    name = "LinalgRelayoutOpsIncGen",
+    tbl_outs = [
+        (
+            ["-gen-op-decls"],
+            "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.h.inc",
+        ),
+        (
+            ["-gen-op-defs"],
+            "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.cpp.inc",
+        ),
+    ],
+    tblgen = ":mlir-tblgen",
+    td_file = "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td",
+    deps = [":LinalgOpsTdFiles"],
+)
+
 gentbl_cc_library(
     name = "LinalgEnumsIncGen",
     tbl_outs = [
@@ -11557,6 +11576,7 @@ cc_library(
         ":LinalgInterfacesIncGen",
         ":LinalgNamedStructuredOpsYamlIncGen",
         ":LinalgOpsIncGen",
+        ":LinalgRelayoutOpsIncGen",
         ":LinalgStructuredOpsIncGen",
         ":MathDialect",
         ":MemRefDialect",
@@ -11568,6 +11588,7 @@ cc_library(
         ":SubsetOpInterface",
         ":Support",
         ":TensorDialect",
+        ":TensorUtils",
         ":TilingInterface",
         ":ValueBoundsOpInterface",
         ":ViewLikeInterface",

@llvmbot
Copy link
Member

llvmbot commented Feb 17, 2025

@llvm/pr-subscribers-mlir-linalg

Author: Christian Sigg (chsigg)

Changes

RelayoutOpInterface is used only to identify tensor.pack/tensor.unpack ops.

This CL moves this interface back to the tensor dialect, to allow the removal of the linalg dialect dependency from the tensor dialect.

This is a partial revert of #125823.


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

9 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Linalg/IR/Linalg.h (+1)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td (-10)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td (+2-1)
  • (modified) mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt (+6)
  • (modified) mlir/include/mlir/Dialect/Tensor/IR/Tensor.h (+6)
  • (added) mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td (+30)
  • (modified) mlir/lib/Dialect/Tensor/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Tensor/IR/TensorOps.cpp (+1-3)
  • (modified) utils/bazel/llvm-project-overlay/mlir/BUILD.bazel (+21)
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h b/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
index 57bf6305a469d..38a8d18e56b05 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
+++ b/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
@@ -10,6 +10,7 @@
 #define MLIR_DIALECT_LINALG_IR_LINALG_H
 
 #include "mlir/Bytecode/BytecodeOpInterface.h"
+#include "mlir/Dialect/Tensor/IR/Tensor.h"
 #include "mlir/Dialect/Utils/ReshapeOpsUtils.h"
 #include "mlir/Dialect/Utils/StructuredOpsUtils.h"
 #include "mlir/IR/AffineExpr.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..2b27f2c908c64 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/Tensor/IR/TensorInterfaces.td"
 include "mlir/IR/OpAsmInterface.td"
 
 //===----------------------------------------------------------------------===//
@@ -32,7 +33,7 @@ include "mlir/IR/OpAsmInterface.td"
 class Linalg_RelayoutOp<string mnemonic, list<Trait> traits = []> :
       Op<Linalg_Dialect, mnemonic, !listconcat(traits, [
         DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>,
-        DestinationStyleOpInterface, LinalgRelayoutOpInterface,
+        DestinationStyleOpInterface, TensorRelayoutOpInterface,
         ConditionallySpeculatable, NoMemoryEffect,
         DeclareOpInterfaceMethods<ReifyRankedShapedTypeOpInterface>,
         TypesMatchWith<"result type matches type of dest",
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
index cd14fe5c04561..74a05291376b3 100644
--- a/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
@@ -1,2 +1,8 @@
 add_mlir_dialect(TensorOps tensor)
 add_mlir_doc(TensorOps TensorOps Dialects/ -gen-dialect-doc)
+
+set(LLVM_TARGET_DEFINITIONS TensorInterfaces.td)
+mlir_tablegen(TensorInterfaces.h.inc -gen-op-interface-decls)
+mlir_tablegen(TensorInterfaces.cpp.inc -gen-op-interface-defs)
+add_public_tablegen_target(MLIRTensorInterfacesIncGen)
+add_dependencies(mlir-headers MLIRTensorInterfacesIncGen)
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h b/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
index eb550bb469b9f..b3ec796a72337 100644
--- a/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
+++ b/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
@@ -46,6 +46,12 @@ SmallVector<Range, 8> getOrCreateRanges(OffsetSizeAndStrideOpInterface op,
 
 #include "mlir/Dialect/Tensor/IR/TensorOpsDialect.h.inc"
 
+//===----------------------------------------------------------------------===//
+// Tensor Interfaces
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Dialect/Tensor/IR/TensorInterfaces.h.inc"
+
 //===----------------------------------------------------------------------===//
 // Tensor Dialect Operations
 //===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td b/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td
new file mode 100644
index 0000000000000..83c173b182664
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td
@@ -0,0 +1,30 @@
+//===- TensorInterfaces.td - Tensor Interfaces 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
+//
+//===----------------------------------------------------------------------===//
+//
+// This is the definition file for the structured interface sfor Tensor ops.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef TENSOR_IR_TENSORINTERFACES
+#define TENSOR_IR_TENSORINTERFACES
+
+include "mlir/Interfaces/DestinationStyleOpInterface.td"
+include "mlir/IR/OpBase.td"
+
+def TensorRelayoutOpInterface : OpInterface<"RelayoutOpInterface"> {
+  let description = [{
+    A Tensor 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::tensor";
+}
+
+#endif // TENSOR_IR_TENSORINTERFACES
+
diff --git a/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt b/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
index 5425615dac393..d9d09d6361a2f 100644
--- a/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
+++ b/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
@@ -16,6 +16,7 @@ add_mlir_dialect_library(MLIRTensorDialect
 
   DEPENDS
   MLIRTensorOpsIncGen
+  MLIRTensorInterfacesIncGen
 
   LINK_LIBS PUBLIC
   MLIRAffineDialect
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index e741144647043..eeef383ac531a 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -10,9 +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/Linalg.h"
 #include "mlir/Dialect/Tensor/IR/Tensor.h"
-#include "mlir/Dialect/Tensor/Utils/Utils.h"
 #include "mlir/Dialect/Utils/IndexingUtils.h"
 #include "mlir/Dialect/Utils/ReshapeOpsUtils.h"
 #include "mlir/Dialect/Utils/StaticValueUtils.h"
@@ -3914,7 +3912,7 @@ struct FoldTensorCastProducerOp
     // Reject PackOp/UnpackOp (i.e. RelayoutOps) - there are dedicated patterns
     // for that instead.
     if (!foldTensorCastPrecondition(op) ||
-        isa<linalg::RelayoutOpInterface>(*op))
+        isa<tensor::RelayoutOpInterface>(*op))
       return failure();
 
     SmallVector<Type> newResultTypes(op->getResultTypes());
diff --git a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
index 92aedac837197..2569847ab6efe 100644
--- a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
@@ -11140,6 +11140,7 @@ td_library(
         "include/mlir/Dialect/Linalg/IR/LinalgEnums.td",
         "include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td",
         "include/mlir/Dialect/Linalg/IR/LinalgOps.td",
+        "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td",
     ],
     includes = ["include"],
     deps = [
@@ -11150,6 +11151,7 @@ td_library(
         ":LoopLikeInterfaceTdFiles",
         ":OpBaseTdFiles",
         ":SideEffectInterfacesTdFiles",
+        ":TensorOpsTdFiles",
         ":TilingInterfaceTdFiles",
         ":ViewLikeInterfaceTdFiles",
     ],
@@ -11206,6 +11208,23 @@ gentbl_cc_library(
     deps = [":LinalgOpsTdFiles"],
 )
 
+gentbl_cc_library(
+    name = "LinalgRelayoutOpsIncGen",
+    tbl_outs = [
+        (
+            ["-gen-op-decls"],
+            "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.h.inc",
+        ),
+        (
+            ["-gen-op-defs"],
+            "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.cpp.inc",
+        ),
+    ],
+    tblgen = ":mlir-tblgen",
+    td_file = "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td",
+    deps = [":LinalgOpsTdFiles"],
+)
+
 gentbl_cc_library(
     name = "LinalgEnumsIncGen",
     tbl_outs = [
@@ -11557,6 +11576,7 @@ cc_library(
         ":LinalgInterfacesIncGen",
         ":LinalgNamedStructuredOpsYamlIncGen",
         ":LinalgOpsIncGen",
+        ":LinalgRelayoutOpsIncGen",
         ":LinalgStructuredOpsIncGen",
         ":MathDialect",
         ":MemRefDialect",
@@ -11568,6 +11588,7 @@ cc_library(
         ":SubsetOpInterface",
         ":Support",
         ":TensorDialect",
+        ":TensorUtils",
         ":TilingInterface",
         ":ValueBoundsOpInterface",
         ":ViewLikeInterface",

@llvmbot
Copy link
Member

llvmbot commented Feb 17, 2025

@llvm/pr-subscribers-mlir

Author: Christian Sigg (chsigg)

Changes

RelayoutOpInterface is used only to identify tensor.pack/tensor.unpack ops.

This CL moves this interface back to the tensor dialect, to allow the removal of the linalg dialect dependency from the tensor dialect.

This is a partial revert of #125823.


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

9 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Linalg/IR/Linalg.h (+1)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td (-10)
  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td (+2-1)
  • (modified) mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt (+6)
  • (modified) mlir/include/mlir/Dialect/Tensor/IR/Tensor.h (+6)
  • (added) mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td (+30)
  • (modified) mlir/lib/Dialect/Tensor/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Tensor/IR/TensorOps.cpp (+1-3)
  • (modified) utils/bazel/llvm-project-overlay/mlir/BUILD.bazel (+21)
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h b/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
index 57bf6305a469d..38a8d18e56b05 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
+++ b/mlir/include/mlir/Dialect/Linalg/IR/Linalg.h
@@ -10,6 +10,7 @@
 #define MLIR_DIALECT_LINALG_IR_LINALG_H
 
 #include "mlir/Bytecode/BytecodeOpInterface.h"
+#include "mlir/Dialect/Tensor/IR/Tensor.h"
 #include "mlir/Dialect/Utils/ReshapeOpsUtils.h"
 #include "mlir/Dialect/Utils/StructuredOpsUtils.h"
 #include "mlir/IR/AffineExpr.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..2b27f2c908c64 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/Tensor/IR/TensorInterfaces.td"
 include "mlir/IR/OpAsmInterface.td"
 
 //===----------------------------------------------------------------------===//
@@ -32,7 +33,7 @@ include "mlir/IR/OpAsmInterface.td"
 class Linalg_RelayoutOp<string mnemonic, list<Trait> traits = []> :
       Op<Linalg_Dialect, mnemonic, !listconcat(traits, [
         DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>,
-        DestinationStyleOpInterface, LinalgRelayoutOpInterface,
+        DestinationStyleOpInterface, TensorRelayoutOpInterface,
         ConditionallySpeculatable, NoMemoryEffect,
         DeclareOpInterfaceMethods<ReifyRankedShapedTypeOpInterface>,
         TypesMatchWith<"result type matches type of dest",
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
index cd14fe5c04561..74a05291376b3 100644
--- a/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/Tensor/IR/CMakeLists.txt
@@ -1,2 +1,8 @@
 add_mlir_dialect(TensorOps tensor)
 add_mlir_doc(TensorOps TensorOps Dialects/ -gen-dialect-doc)
+
+set(LLVM_TARGET_DEFINITIONS TensorInterfaces.td)
+mlir_tablegen(TensorInterfaces.h.inc -gen-op-interface-decls)
+mlir_tablegen(TensorInterfaces.cpp.inc -gen-op-interface-defs)
+add_public_tablegen_target(MLIRTensorInterfacesIncGen)
+add_dependencies(mlir-headers MLIRTensorInterfacesIncGen)
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h b/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
index eb550bb469b9f..b3ec796a72337 100644
--- a/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
+++ b/mlir/include/mlir/Dialect/Tensor/IR/Tensor.h
@@ -46,6 +46,12 @@ SmallVector<Range, 8> getOrCreateRanges(OffsetSizeAndStrideOpInterface op,
 
 #include "mlir/Dialect/Tensor/IR/TensorOpsDialect.h.inc"
 
+//===----------------------------------------------------------------------===//
+// Tensor Interfaces
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Dialect/Tensor/IR/TensorInterfaces.h.inc"
+
 //===----------------------------------------------------------------------===//
 // Tensor Dialect Operations
 //===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td b/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td
new file mode 100644
index 0000000000000..83c173b182664
--- /dev/null
+++ b/mlir/include/mlir/Dialect/Tensor/IR/TensorInterfaces.td
@@ -0,0 +1,30 @@
+//===- TensorInterfaces.td - Tensor Interfaces 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
+//
+//===----------------------------------------------------------------------===//
+//
+// This is the definition file for the structured interface sfor Tensor ops.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef TENSOR_IR_TENSORINTERFACES
+#define TENSOR_IR_TENSORINTERFACES
+
+include "mlir/Interfaces/DestinationStyleOpInterface.td"
+include "mlir/IR/OpBase.td"
+
+def TensorRelayoutOpInterface : OpInterface<"RelayoutOpInterface"> {
+  let description = [{
+    A Tensor 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::tensor";
+}
+
+#endif // TENSOR_IR_TENSORINTERFACES
+
diff --git a/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt b/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
index 5425615dac393..d9d09d6361a2f 100644
--- a/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
+++ b/mlir/lib/Dialect/Tensor/IR/CMakeLists.txt
@@ -16,6 +16,7 @@ add_mlir_dialect_library(MLIRTensorDialect
 
   DEPENDS
   MLIRTensorOpsIncGen
+  MLIRTensorInterfacesIncGen
 
   LINK_LIBS PUBLIC
   MLIRAffineDialect
diff --git a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
index e741144647043..eeef383ac531a 100644
--- a/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
+++ b/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
@@ -10,9 +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/Linalg.h"
 #include "mlir/Dialect/Tensor/IR/Tensor.h"
-#include "mlir/Dialect/Tensor/Utils/Utils.h"
 #include "mlir/Dialect/Utils/IndexingUtils.h"
 #include "mlir/Dialect/Utils/ReshapeOpsUtils.h"
 #include "mlir/Dialect/Utils/StaticValueUtils.h"
@@ -3914,7 +3912,7 @@ struct FoldTensorCastProducerOp
     // Reject PackOp/UnpackOp (i.e. RelayoutOps) - there are dedicated patterns
     // for that instead.
     if (!foldTensorCastPrecondition(op) ||
-        isa<linalg::RelayoutOpInterface>(*op))
+        isa<tensor::RelayoutOpInterface>(*op))
       return failure();
 
     SmallVector<Type> newResultTypes(op->getResultTypes());
diff --git a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
index 92aedac837197..2569847ab6efe 100644
--- a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
@@ -11140,6 +11140,7 @@ td_library(
         "include/mlir/Dialect/Linalg/IR/LinalgEnums.td",
         "include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td",
         "include/mlir/Dialect/Linalg/IR/LinalgOps.td",
+        "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td",
     ],
     includes = ["include"],
     deps = [
@@ -11150,6 +11151,7 @@ td_library(
         ":LoopLikeInterfaceTdFiles",
         ":OpBaseTdFiles",
         ":SideEffectInterfacesTdFiles",
+        ":TensorOpsTdFiles",
         ":TilingInterfaceTdFiles",
         ":ViewLikeInterfaceTdFiles",
     ],
@@ -11206,6 +11208,23 @@ gentbl_cc_library(
     deps = [":LinalgOpsTdFiles"],
 )
 
+gentbl_cc_library(
+    name = "LinalgRelayoutOpsIncGen",
+    tbl_outs = [
+        (
+            ["-gen-op-decls"],
+            "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.h.inc",
+        ),
+        (
+            ["-gen-op-defs"],
+            "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.cpp.inc",
+        ),
+    ],
+    tblgen = ":mlir-tblgen",
+    td_file = "include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td",
+    deps = [":LinalgOpsTdFiles"],
+)
+
 gentbl_cc_library(
     name = "LinalgEnumsIncGen",
     tbl_outs = [
@@ -11557,6 +11576,7 @@ cc_library(
         ":LinalgInterfacesIncGen",
         ":LinalgNamedStructuredOpsYamlIncGen",
         ":LinalgOpsIncGen",
+        ":LinalgRelayoutOpsIncGen",
         ":LinalgStructuredOpsIncGen",
         ":MathDialect",
         ":MemRefDialect",
@@ -11568,6 +11588,7 @@ cc_library(
         ":SubsetOpInterface",
         ":Support",
         ":TensorDialect",
+        ":TensorUtils",
         ":TilingInterface",
         ":ValueBoundsOpInterface",
         ":ViewLikeInterface",

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.

I don't understand what you're trying to do. Adding an interface to the tensor dialect that only operations in the linalg dialect use makes no sense.

What is the dependency that you mention between tensor and linalg? Please, work with @banach-space to resolve conflicts instead of partially reverting previous commits.

@banach-space
Copy link
Contributor

banach-space commented Feb 17, 2025

RelayoutOpInterface is used only to identify tensor.pack/tensor.unpack ops.

Please note that tensor.pack + tensor.unpack have been moved to Linalg. With this interface, Tensor depends on the interface rather than the whole dialect. I discuss this more in this PR:

What is the dependency that you mention between tensor and linalg?

That's needed here:

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

@rengolin , I assumed that a dependency on an interface is fine. From https://mlir.llvm.org/docs/Interfaces/:

Interfaces provide a generic way of interacting with the IR. The goal is to be able to express transformations/analyses in terms of these interfaces without encoding specific knowledge about the exact operation or dialect involved. This makes the compiler more easily extensible by allowing the addition of new dialects and operations in a decoupled way with respect to the implementation of transformations/analyses.

What's your interpretation of that paragraph? Is there a better mechanism to disable that pattern for Linalg Ops?

@rengolin
Copy link
Member

What's your interpretation of that paragraph? Is there a better mechanism to disable that pattern for Linalg Ops?

Previously, the tensor dialect transforms (on pack and unpack) depended on a lot more than an interface, as it lowered those ops to linalg ops and therefore needed to bring also implementation and everything else with it.

If the interface is in a header that does not require implementation details, then it's a reasonable way to share information without actually requiring one to link all object files unless using the actual dialect.

So the question is: why is this showing on @chsigg build? I'm assuming you're not using linalg, but still got linker errors, even though the interface should not have done that. So, if you can show us the error, we can fix the dependency in the right way.

@banach-space
Copy link
Contributor

If the interface is in a header that does not require implementation details, then it's a reasonable way to share information without actually requiring one to link all object files unless using the actual dialect.

It's indeed a header file, but it's TableGen'ed, hence "build system" dependency. I suspect that's the undesired side-effect?

@chsigg
Copy link
Contributor Author

chsigg commented Feb 17, 2025

Thanks for your feedback. I'm trying to fix the bazel build. However, I cannot add LinalgDialect as a dependency of TensorDialect, because it would create a circle (LinalgDialect already depends on TensorDialect).

A potential alternative route would be to expose the RelayoutOpInterface in a separate header (so that it can become its own self-contained target). I'm aware that bazel builds are not officially supported and if need be we can come up with creative ways to write BUILD files to handle the current code. We would have targets that expose Linalg.h but only implement a subset of what the header declares. We have done this before, but it's a can of worms and we made an effort to fix these a while back (see e.g. #86637).

I'm not familiar with the (cmake) build, so I can't tell if the current form has downsides there as well. Like, do users of the tensor dialect now need to build all of linalg, but didn't need to before? And if it were the case, is it an issue?

@chsigg
Copy link
Contributor Author

chsigg commented Feb 17, 2025

If the interface is in a header that does not require implementation details, then it's a reasonable way to share information without actually requiring one to link all object files unless using the actual dialect.

It's indeed a header file, but it's TableGen'ed, hence "build system" dependency. I suspect that's the undesired side-effect?

The undesired part is that it's only exposed in the 'top-level' dialect header Linalg.h. If it were exposed in a separate header (that Linalg.h is free to include), it would be much easier to handle from a bazel build perspective.

@rengolin
Copy link
Member

Thanks for your feedback. I'm trying to fix the bazel build. However, I cannot add LinalgDialect as a dependency of TensorDialect, because it would create a circle (LinalgDialect already depends on TensorDialect).

Side note: please do not propose radical code changes to fix the Bazel build. Bazel is in the peripheral tier in the LLVM support policy (https://llvm.org/docs/SupportPolicy.html#peripheral-tier) and as such, must not be the sole reason for reverting a patch.

I'm not familiar with the (cmake) build, so I can't tell if the current form has downsides there as well. Like, do users of the tensor dialect now need to build all of linalg, but didn't need to before? And if it were the case, is it an issue?

This is what we are trying to fix. Up until now, the tensor dialect had a direct dependency with linalg, and the PR that you're trying to revert is a direct fix for that problem (plus the other surrounding PRs by @banach-space). I'm surprised this broke stuff for Bazel, given that it was already a harder dependency.

It's indeed a header file, but it's TableGen'ed, hence "build system" dependency. I suspect that's the undesired side-effect?

The undesired part is that it's only exposed in the 'top-level' dialect header Linalg.h. If it were exposed in a separate header (that Linalg.h is free to include), it would be much easier to handle from a bazel build perspective.

These two statements are at odds. If it's a table-gen'ed file, it's not Linalg.h. I imagine the Tensor side would just include the .inc directly?

@chsigg
Copy link
Contributor Author

chsigg commented Feb 17, 2025

I'm aware that bazel builds are not officially supported
please do not propose radical code changes to fix the Bazel build.

And I asked whether it might also not be ideal for cmake, but I'm not familiar with that build.

Up until now, the tensor dialect had a direct dependency with linalg,

That is, the Linalg dialect depends on various things from Tensor, including several op definitions. #123902 introduced a dependency the other way around, where the tensor ops depend on linalg.

@rengolin
Copy link
Member

I'm aware that bazel builds are not officially supported
please do not propose radical code changes to fix the Bazel build.

And I asked whether it might also not be ideal for cmake, but I'm not familiar with that build.

Perhaps starting with a question instead of a revert PR would have been better. PRs have the tendency to being missed by some, approved by others, and by the time you realize your patch has been reverted, many things have changed and you have to start from scratch.

Up until now, the tensor dialect had a direct dependency with linalg,

That is, the Linalg dialect depends on various things from Tensor, including several op definitions. #123902 introduced a dependency the other way around, where the tensor ops depend on linalg.

No, I mean, the lowering of the previous tensor.pack operation involved using linalg.fill, which was the main reason we moved the ops across. I'm surprised this wasn't a dependency for Bazel until now.

@chsigg
Copy link
Contributor Author

chsigg commented Feb 17, 2025

These two statements are at odds. If it's a table-gen'ed file, it's not Linalg.h. I imagine the Tensor side would just include the .inc directly?

This would be quite uncommon, we don't include .inc files across dialect boundaries because they are not self-contained (they usually need other includes before them or even some defines).

But I still missed something, the LinalgInterfaces.h.inc is actually exposed in LinalgInterfaces.h, not the top-level Linalg.h. I'm sorry about that, I got confused between RelayoutOpsInterface and RelayoutOps.

I will close this PR and instead restructure the bazel BUILD files to split out a separate LinalgInterfaces target. I think this should be doable.

@chsigg chsigg closed this Feb 17, 2025
banach-space added a commit to banach-space/llvm-project that referenced this pull request Feb 17, 2025
@banach-space
Copy link
Contributor

It's indeed a header file, but it's TableGen'ed, hence "build system" dependency. I suspect that's the undesired side-effect?

The undesired part is that it's only exposed in the 'top-level' dialect header Linalg.h. If it were exposed in a separate header (that Linalg.h is free to include), it would be much easier to handle from a bazel build perspective.

These two statements are at odds. If it's a table-gen'ed file, it's not Linalg.h. I imagine the Tensor side would just include the .inc directly?

Yes, that's possible:

@chsigg , would that make the Bazel side of things easier for you?

@chsigg
Copy link
Contributor Author

chsigg commented Feb 17, 2025

If everything goes well, I will only need to change this include from Linalg.h to LinalgInterfaces.h.

@rengolin
Copy link
Member

If everything goes well, I will only need to change this include from Linalg.h to LinalgInterfaces.h.

Awesome! I think @banach-space's new PR does that for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel mlir:linalg mlir:tensor mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants