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] Move InlinerInterface from Transforms to Interfaces. #84878

Closed
wants to merge 1 commit into from

Conversation

chsigg
Copy link
Contributor

@chsigg chsigg commented Mar 12, 2024

Most interfaces live in the mlir/Interfaces directory, but the InlinerInterface was defined (I think for historical reasons) in mlir/Transforms/Utils/InliningUtils.h.

Telling from the bazel BUILD files (I'm not familiar with the CMake build), this created circular dependencies which required various dialect targets to depend on InliningUtils.h. I was trying to clean this up, and the best solution seems to be to move the InlinerInterface from the Transforms directory to Interfaces.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 12, 2024

@llvm/pr-subscribers-mlir-shape
@llvm/pr-subscribers-mlir-func
@llvm/pr-subscribers-mlir-spirv
@llvm/pr-subscribers-mlir-arith
@llvm/pr-subscribers-mlir-affine
@llvm/pr-subscribers-mlir-tensor
@llvm/pr-subscribers-mlir-vector
@llvm/pr-subscribers-mlir-gpu
@llvm/pr-subscribers-mlir-cf
@llvm/pr-subscribers-mlir-bufferization
@llvm/pr-subscribers-mlir-memref
@llvm/pr-subscribers-mlir-linalg

@llvm/pr-subscribers-mlir-llvm

Author: Christian Sigg (chsigg)

Changes

Patch is 60.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84878.diff

46 Files Affected:

  • (added) mlir/include/mlir/Interfaces/InlinerInterface.h (+221)
  • (modified) mlir/include/mlir/Transforms/InliningUtils.h (+1-204)
  • (modified) mlir/lib/Dialect/Affine/IR/AffineOps.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Affine/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Arith/IR/ArithDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Arith/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Bufferization/IR/BufferizationDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Bufferization/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Complex/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Complex/IR/ComplexDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/ControlFlow/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/ControlFlow/IR/ControlFlowOps.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Func/Extensions/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Func/Extensions/InlinerExtension.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Func/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Func/IR/FuncOps.cpp (+1-1)
  • (modified) mlir/lib/Dialect/GPU/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/GPU/IR/GPUDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/LLVMIR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/LLVMIR/IR/LLVMInlining.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Linalg/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Math/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Math/IR/MathDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/MemRef/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/MemRef/IR/MemRefDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/SCF/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/SCF/IR/SCF.cpp (+1-1)
  • (modified) mlir/lib/Dialect/SPIRV/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Shape/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Shape/IR/Shape.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Tensor/IR/CMakeLists.txt (+2-1)
  • (modified) mlir/lib/Dialect/Tensor/IR/TensorDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/UB/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/UB/IR/UBOps.cpp (+2-2)
  • (modified) mlir/lib/Dialect/Vector/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Vector/IR/VectorOps.cpp (+1-1)
  • (modified) mlir/lib/Interfaces/CMakeLists.txt (+11)
  • (added) mlir/lib/Interfaces/InlinerInterface.cpp (+91)
  • (modified) mlir/lib/Transforms/CMakeLists.txt (+1)
  • (modified) mlir/lib/Transforms/Utils/CMakeLists.txt (+1)
  • (modified) mlir/lib/Transforms/Utils/InliningUtils.cpp (-73)
  • (modified) mlir/test/lib/Dialect/Test/TestDialect.cpp (+1-1)
  • (modified) mlir/test/lib/Dialect/Test/TestDialectInterfaces.cpp (+1-1)
  • (modified) utils/bazel/llvm-project-overlay/mlir/BUILD.bazel (+33-9)
  • (modified) utils/bazel/llvm-project-overlay/mlir/test/BUILD.bazel (+11-13)
diff --git a/mlir/include/mlir/Interfaces/InlinerInterface.h b/mlir/include/mlir/Interfaces/InlinerInterface.h
new file mode 100644
index 00000000000000..1caee6c785f495
--- /dev/null
+++ b/mlir/include/mlir/Interfaces/InlinerInterface.h
@@ -0,0 +1,221 @@
+//===- InlinerInterface.h - Inliner interface -------------------*- C++ -*-===//
+//
+// 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 header file defines interfaces for various inlining utility methods.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_INTERFACES_INLINERINTERFACE_H
+#define MLIR_INTERFACES_INLINERINTERFACE_H
+
+#include "mlir/IR/BuiltinAttributes.h"
+#include "mlir/IR/DialectInterface.h"
+#include "mlir/IR/Location.h"
+#include "mlir/IR/Region.h"
+#include "mlir/IR/ValueRange.h"
+
+namespace mlir {
+
+class IRMapping;
+class OpBuilder;
+
+//===----------------------------------------------------------------------===//
+// InlinerInterface
+//===----------------------------------------------------------------------===//
+
+/// This is the interface that must be implemented by the dialects of operations
+/// to be inlined. This interface should only handle the operations of the
+/// given dialect.
+class DialectInlinerInterface
+    : public DialectInterface::Base<DialectInlinerInterface> {
+public:
+  DialectInlinerInterface(Dialect *dialect) : Base(dialect) {}
+
+  //===--------------------------------------------------------------------===//
+  // Analysis Hooks
+  //===--------------------------------------------------------------------===//
+
+  /// Returns true if the given operation 'callable', that implements the
+  /// 'CallableOpInterface', can be inlined into the position given call
+  /// operation 'call', that is registered to the current dialect and implements
+  /// the `CallOpInterface`. 'wouldBeCloned' is set to true if the region of the
+  /// given 'callable' is set to be cloned during the inlining process, or false
+  /// if the region is set to be moved in-place(i.e. no duplicates would be
+  /// created).
+  virtual bool isLegalToInline(Operation *call, Operation *callable,
+                               bool wouldBeCloned) const {
+    return false;
+  }
+
+  /// Returns true if the given region 'src' can be inlined into the region
+  /// 'dest' that is attached to an operation registered to the current dialect.
+  /// 'wouldBeCloned' is set to true if the given 'src' region is set to be
+  /// cloned during the inlining process, or false if the region is set to be
+  /// moved in-place(i.e. no duplicates would be created). 'valueMapping'
+  /// contains any remapped values from within the 'src' region. This can be
+  /// used to examine what values will replace entry arguments into the 'src'
+  /// region for example.
+  virtual bool isLegalToInline(Region *dest, Region *src, bool wouldBeCloned,
+                               IRMapping &valueMapping) const {
+    return false;
+  }
+
+  /// Returns true if the given operation 'op', that is registered to this
+  /// dialect, can be inlined into the given region, false otherwise.
+  /// 'wouldBeCloned' is set to true if the given 'op' is set to be cloned
+  /// during the inlining process, or false if the operation is set to be moved
+  /// in-place(i.e. no duplicates would be created). 'valueMapping' contains any
+  /// remapped values from within the 'src' region. This can be used to examine
+  /// what values may potentially replace the operands to 'op'.
+  virtual bool isLegalToInline(Operation *op, Region *dest, bool wouldBeCloned,
+                               IRMapping &valueMapping) const {
+    return false;
+  }
+
+  /// This hook is invoked on an operation that contains regions. It should
+  /// return true if the analyzer should recurse within the regions of this
+  /// operation when computing legality and cost, false otherwise. The default
+  /// implementation returns true.
+  virtual bool shouldAnalyzeRecursively(Operation *op) const { return true; }
+
+  //===--------------------------------------------------------------------===//
+  // Transformation Hooks
+  //===--------------------------------------------------------------------===//
+
+  /// Handle the given inlined terminator by replacing it with a new operation
+  /// as necessary. This overload is called when the inlined region has more
+  /// than one block. The 'newDest' block represents the new final branching
+  /// destination of blocks within this region, i.e. operations that release
+  /// control to the parent operation will likely now branch to this block.
+  /// Its block arguments correspond to any values that need to be replaced by
+  /// terminators within the inlined region.
+  virtual void handleTerminator(Operation *op, Block *newDest) const {
+    llvm_unreachable("must implement handleTerminator in the case of multiple "
+                     "inlined blocks");
+  }
+
+  /// Handle the given inlined terminator by replacing it with a new operation
+  /// as necessary. This overload is called when the inlined region only
+  /// contains one block. 'valuesToReplace' contains the previously returned
+  /// values of the call site before inlining. These values must be replaced by
+  /// this callback if they had any users (for example for traditional function
+  /// calls, these are directly replaced with the operands of the `return`
+  /// operation). The given 'op' will be removed by the caller, after this
+  /// function has been called.
+  virtual void handleTerminator(Operation *op,
+                                ValueRange valuesToReplace) const {
+    llvm_unreachable(
+        "must implement handleTerminator in the case of one inlined block");
+  }
+
+  /// Attempt to materialize a conversion for a type mismatch between a call
+  /// from this dialect, and a callable region. This method should generate an
+  /// operation that takes 'input' as the only operand, and produces a single
+  /// result of 'resultType'. If a conversion can not be generated, nullptr
+  /// should be returned. For example, this hook may be invoked in the following
+  /// scenarios:
+  ///   func @foo(i32) -> i32 { ... }
+  ///
+  ///   // Mismatched input operand
+  ///   ... = foo.call @foo(%input : i16) -> i32
+  ///
+  ///   // Mismatched result type.
+  ///   ... = foo.call @foo(%input : i32) -> i16
+  ///
+  /// NOTE: This hook may be invoked before the 'isLegal' checks above.
+  virtual Operation *materializeCallConversion(OpBuilder &builder, Value input,
+                                               Type resultType,
+                                               Location conversionLoc) const {
+    return nullptr;
+  }
+
+  /// Hook to transform the call arguments before using them to replace the
+  /// callee arguments. Returns a value of the same type or the `argument`
+  /// itself if nothing changed. The `argumentAttrs` dictionary is non-null even
+  /// if no attribute is present. The hook is called after converting the
+  /// callsite argument types using the materializeCallConversion callback, and
+  /// right before inlining the callee region. Any operations created using the
+  /// provided `builder` are inserted right before the inlined callee region. An
+  /// example use case is the insertion of copies for by value arguments.
+  virtual Value handleArgument(OpBuilder &builder, Operation *call,
+                               Operation *callable, Value argument,
+                               DictionaryAttr argumentAttrs) const {
+    return argument;
+  }
+
+  /// Hook to transform the callee results before using them to replace the call
+  /// results. Returns a value of the same type or the `result` itself if
+  /// nothing changed. The `resultAttrs` dictionary is non-null even if no
+  /// attribute is present. The hook is called right before handling
+  /// terminators, and obtains the callee result before converting its type
+  /// using the `materializeCallConversion` callback. Any operations created
+  /// using the provided `builder` are inserted right after the inlined callee
+  /// region. An example use case is the insertion of copies for by value
+  /// results. NOTE: This hook is invoked after inlining the `callable` region.
+  virtual Value handleResult(OpBuilder &builder, Operation *call,
+                             Operation *callable, Value result,
+                             DictionaryAttr resultAttrs) const {
+    return result;
+  }
+
+  /// Process a set of blocks that have been inlined for a call. This callback
+  /// is invoked before inlined terminator operations have been processed.
+  virtual void processInlinedCallBlocks(
+      Operation *call, iterator_range<Region::iterator> inlinedBlocks) const {}
+};
+
+/// This interface provides the hooks into the inlining interface.
+/// Note: this class automatically collects 'DialectInlinerInterface' objects
+/// registered to each dialect within the given context.
+class InlinerInterface
+    : public DialectInterfaceCollection<DialectInlinerInterface> {
+public:
+  using Base::Base;
+
+  /// Process a set of blocks that have been inlined. This callback is invoked
+  /// *before* inlined terminator operations have been processed.
+  virtual void
+  processInlinedBlocks(iterator_range<Region::iterator> inlinedBlocks) {}
+
+  /// These hooks mirror the hooks for the DialectInlinerInterface, with default
+  /// implementations that call the hook on the handler for the dialect 'op' is
+  /// registered to.
+
+  //===--------------------------------------------------------------------===//
+  // Analysis Hooks
+  //===--------------------------------------------------------------------===//
+
+  virtual bool isLegalToInline(Operation *call, Operation *callable,
+                               bool wouldBeCloned) const;
+  virtual bool isLegalToInline(Region *dest, Region *src, bool wouldBeCloned,
+                               IRMapping &valueMapping) const;
+  virtual bool isLegalToInline(Operation *op, Region *dest, bool wouldBeCloned,
+                               IRMapping &valueMapping) const;
+  virtual bool shouldAnalyzeRecursively(Operation *op) const;
+
+  //===--------------------------------------------------------------------===//
+  // Transformation Hooks
+  //===--------------------------------------------------------------------===//
+
+  virtual void handleTerminator(Operation *op, Block *newDest) const;
+  virtual void handleTerminator(Operation *op, ValueRange valuesToRepl) const;
+
+  virtual Value handleArgument(OpBuilder &builder, Operation *call,
+                               Operation *callable, Value argument,
+                               DictionaryAttr argumentAttrs) const;
+  virtual Value handleResult(OpBuilder &builder, Operation *call,
+                             Operation *callable, Value result,
+                             DictionaryAttr resultAttrs) const;
+
+  virtual void processInlinedCallBlocks(
+      Operation *call, iterator_range<Region::iterator> inlinedBlocks) const;
+};
+
+} // namespace mlir
+
+#endif // MLIR_INTERFACES_INLINERINTERFACE_H
diff --git a/mlir/include/mlir/Transforms/InliningUtils.h b/mlir/include/mlir/Transforms/InliningUtils.h
index 88fc033a6ab7be..16d12075fee9c4 100644
--- a/mlir/include/mlir/Transforms/InliningUtils.h
+++ b/mlir/include/mlir/Transforms/InliningUtils.h
@@ -13,217 +13,14 @@
 #ifndef MLIR_TRANSFORMS_INLININGUTILS_H
 #define MLIR_TRANSFORMS_INLININGUTILS_H
 
-#include "mlir/IR/BuiltinAttributes.h"
-#include "mlir/IR/DialectInterface.h"
-#include "mlir/IR/Location.h"
-#include "mlir/IR/Region.h"
 #include "mlir/IR/ValueRange.h"
+#include "mlir/Interfaces/InlinerInterface.h"
 #include <optional>
 
 namespace mlir {
 
-class Block;
-class IRMapping;
 class CallableOpInterface;
 class CallOpInterface;
-class OpBuilder;
-class Operation;
-class Region;
-class TypeRange;
-class Value;
-class ValueRange;
-
-//===----------------------------------------------------------------------===//
-// InlinerInterface
-//===----------------------------------------------------------------------===//
-
-/// This is the interface that must be implemented by the dialects of operations
-/// to be inlined. This interface should only handle the operations of the
-/// given dialect.
-class DialectInlinerInterface
-    : public DialectInterface::Base<DialectInlinerInterface> {
-public:
-  DialectInlinerInterface(Dialect *dialect) : Base(dialect) {}
-
-  //===--------------------------------------------------------------------===//
-  // Analysis Hooks
-  //===--------------------------------------------------------------------===//
-
-  /// Returns true if the given operation 'callable', that implements the
-  /// 'CallableOpInterface', can be inlined into the position given call
-  /// operation 'call', that is registered to the current dialect and implements
-  /// the `CallOpInterface`. 'wouldBeCloned' is set to true if the region of the
-  /// given 'callable' is set to be cloned during the inlining process, or false
-  /// if the region is set to be moved in-place(i.e. no duplicates would be
-  /// created).
-  virtual bool isLegalToInline(Operation *call, Operation *callable,
-                               bool wouldBeCloned) const {
-    return false;
-  }
-
-  /// Returns true if the given region 'src' can be inlined into the region
-  /// 'dest' that is attached to an operation registered to the current dialect.
-  /// 'wouldBeCloned' is set to true if the given 'src' region is set to be
-  /// cloned during the inlining process, or false if the region is set to be
-  /// moved in-place(i.e. no duplicates would be created). 'valueMapping'
-  /// contains any remapped values from within the 'src' region. This can be
-  /// used to examine what values will replace entry arguments into the 'src'
-  /// region for example.
-  virtual bool isLegalToInline(Region *dest, Region *src, bool wouldBeCloned,
-                               IRMapping &valueMapping) const {
-    return false;
-  }
-
-  /// Returns true if the given operation 'op', that is registered to this
-  /// dialect, can be inlined into the given region, false otherwise.
-  /// 'wouldBeCloned' is set to true if the given 'op' is set to be cloned
-  /// during the inlining process, or false if the operation is set to be moved
-  /// in-place(i.e. no duplicates would be created). 'valueMapping' contains any
-  /// remapped values from within the 'src' region. This can be used to examine
-  /// what values may potentially replace the operands to 'op'.
-  virtual bool isLegalToInline(Operation *op, Region *dest, bool wouldBeCloned,
-                               IRMapping &valueMapping) const {
-    return false;
-  }
-
-  /// This hook is invoked on an operation that contains regions. It should
-  /// return true if the analyzer should recurse within the regions of this
-  /// operation when computing legality and cost, false otherwise. The default
-  /// implementation returns true.
-  virtual bool shouldAnalyzeRecursively(Operation *op) const { return true; }
-
-  //===--------------------------------------------------------------------===//
-  // Transformation Hooks
-  //===--------------------------------------------------------------------===//
-
-  /// Handle the given inlined terminator by replacing it with a new operation
-  /// as necessary. This overload is called when the inlined region has more
-  /// than one block. The 'newDest' block represents the new final branching
-  /// destination of blocks within this region, i.e. operations that release
-  /// control to the parent operation will likely now branch to this block.
-  /// Its block arguments correspond to any values that need to be replaced by
-  /// terminators within the inlined region.
-  virtual void handleTerminator(Operation *op, Block *newDest) const {
-    llvm_unreachable("must implement handleTerminator in the case of multiple "
-                     "inlined blocks");
-  }
-
-  /// Handle the given inlined terminator by replacing it with a new operation
-  /// as necessary. This overload is called when the inlined region only
-  /// contains one block. 'valuesToReplace' contains the previously returned
-  /// values of the call site before inlining. These values must be replaced by
-  /// this callback if they had any users (for example for traditional function
-  /// calls, these are directly replaced with the operands of the `return`
-  /// operation). The given 'op' will be removed by the caller, after this
-  /// function has been called.
-  virtual void handleTerminator(Operation *op,
-                                ValueRange valuesToReplace) const {
-    llvm_unreachable(
-        "must implement handleTerminator in the case of one inlined block");
-  }
-
-  /// Attempt to materialize a conversion for a type mismatch between a call
-  /// from this dialect, and a callable region. This method should generate an
-  /// operation that takes 'input' as the only operand, and produces a single
-  /// result of 'resultType'. If a conversion can not be generated, nullptr
-  /// should be returned. For example, this hook may be invoked in the following
-  /// scenarios:
-  ///   func @foo(i32) -> i32 { ... }
-  ///
-  ///   // Mismatched input operand
-  ///   ... = foo.call @foo(%input : i16) -> i32
-  ///
-  ///   // Mismatched result type.
-  ///   ... = foo.call @foo(%input : i32) -> i16
-  ///
-  /// NOTE: This hook may be invoked before the 'isLegal' checks above.
-  virtual Operation *materializeCallConversion(OpBuilder &builder, Value input,
-                                               Type resultType,
-                                               Location conversionLoc) const {
-    return nullptr;
-  }
-
-  /// Hook to transform the call arguments before using them to replace the
-  /// callee arguments. Returns a value of the same type or the `argument`
-  /// itself if nothing changed. The `argumentAttrs` dictionary is non-null even
-  /// if no attribute is present. The hook is called after converting the
-  /// callsite argument types using the materializeCallConversion callback, and
-  /// right before inlining the callee region. Any operations created using the
-  /// provided `builder` are inserted right before the inlined callee region. An
-  /// example use case is the insertion of copies for by value arguments.
-  virtual Value handleArgument(OpBuilder &builder, Operation *call,
-                               Operation *callable, Value argument,
-                               DictionaryAttr argumentAttrs) const {
-    return argument;
-  }
-
-  /// Hook to transform the callee results before using them to replace the call
-  /// results. Returns a value of the same type or the `result` itself if
-  /// nothing changed. The `resultAttrs` dictionary is non-null even if no
-  /// attribute is present. The hook is called right before handling
-  /// terminators, and obtains the callee result before converting its type
-  /// using the `materializeCallConversion` callback. Any operations created
-  /// using the provided `builder` are inserted right after the inlined callee
-  /// region. An example use case is the insertion of copies for by value
-  /// results. NOTE: This hook is invoked after inlining the `callable` region.
-  virtual Value handleResult(OpBuilder &builder, Operation *call,
-                             Operation *callable, Value result,
-                             DictionaryAttr resultAttrs) const {
-    return result;
-  }
-
-  /// Process a set of blocks that have been inlined for a call. This callback
-  /// is invoked before inlined terminator operations have been processed.
-  virtual void processInlinedCallBlocks(
-      Operation *call, iterator_range<Region::iterator> inlinedBlocks) const {}
-};
-
-/// This interface provides the hooks into the inlining interface.
-/// Note: this class automatically collects 'DialectInlinerInterface' objects
-/// registered to each dialect within the given context.
-class InlinerInterface
-    : public DialectInterfaceCollection<DialectInlinerInterface> {
-public:
-  using Base::Base;
-...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 12, 2024

@llvm/pr-subscribers-mlir-core

Author: Christian Sigg (chsigg)

Changes

Patch is 60.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84878.diff

46 Files Affected:

  • (added) mlir/include/mlir/Interfaces/InlinerInterface.h (+221)
  • (modified) mlir/include/mlir/Transforms/InliningUtils.h (+1-204)
  • (modified) mlir/lib/Dialect/Affine/IR/AffineOps.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Affine/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Arith/IR/ArithDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Arith/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Bufferization/IR/BufferizationDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Bufferization/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Complex/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Complex/IR/ComplexDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/ControlFlow/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/ControlFlow/IR/ControlFlowOps.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Func/Extensions/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Func/Extensions/InlinerExtension.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Func/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Func/IR/FuncOps.cpp (+1-1)
  • (modified) mlir/lib/Dialect/GPU/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/GPU/IR/GPUDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/LLVMIR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/LLVMIR/IR/LLVMInlining.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Linalg/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Math/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Math/IR/MathDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/MemRef/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/MemRef/IR/MemRefDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/SCF/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/SCF/IR/SCF.cpp (+1-1)
  • (modified) mlir/lib/Dialect/SPIRV/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Shape/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Shape/IR/Shape.cpp (+1-1)
  • (modified) mlir/lib/Dialect/Tensor/IR/CMakeLists.txt (+2-1)
  • (modified) mlir/lib/Dialect/Tensor/IR/TensorDialect.cpp (+1-1)
  • (modified) mlir/lib/Dialect/UB/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/UB/IR/UBOps.cpp (+2-2)
  • (modified) mlir/lib/Dialect/Vector/IR/CMakeLists.txt (+1)
  • (modified) mlir/lib/Dialect/Vector/IR/VectorOps.cpp (+1-1)
  • (modified) mlir/lib/Interfaces/CMakeLists.txt (+11)
  • (added) mlir/lib/Interfaces/InlinerInterface.cpp (+91)
  • (modified) mlir/lib/Transforms/CMakeLists.txt (+1)
  • (modified) mlir/lib/Transforms/Utils/CMakeLists.txt (+1)
  • (modified) mlir/lib/Transforms/Utils/InliningUtils.cpp (-73)
  • (modified) mlir/test/lib/Dialect/Test/TestDialect.cpp (+1-1)
  • (modified) mlir/test/lib/Dialect/Test/TestDialectInterfaces.cpp (+1-1)
  • (modified) utils/bazel/llvm-project-overlay/mlir/BUILD.bazel (+33-9)
  • (modified) utils/bazel/llvm-project-overlay/mlir/test/BUILD.bazel (+11-13)
diff --git a/mlir/include/mlir/Interfaces/InlinerInterface.h b/mlir/include/mlir/Interfaces/InlinerInterface.h
new file mode 100644
index 00000000000000..1caee6c785f495
--- /dev/null
+++ b/mlir/include/mlir/Interfaces/InlinerInterface.h
@@ -0,0 +1,221 @@
+//===- InlinerInterface.h - Inliner interface -------------------*- C++ -*-===//
+//
+// 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 header file defines interfaces for various inlining utility methods.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_INTERFACES_INLINERINTERFACE_H
+#define MLIR_INTERFACES_INLINERINTERFACE_H
+
+#include "mlir/IR/BuiltinAttributes.h"
+#include "mlir/IR/DialectInterface.h"
+#include "mlir/IR/Location.h"
+#include "mlir/IR/Region.h"
+#include "mlir/IR/ValueRange.h"
+
+namespace mlir {
+
+class IRMapping;
+class OpBuilder;
+
+//===----------------------------------------------------------------------===//
+// InlinerInterface
+//===----------------------------------------------------------------------===//
+
+/// This is the interface that must be implemented by the dialects of operations
+/// to be inlined. This interface should only handle the operations of the
+/// given dialect.
+class DialectInlinerInterface
+    : public DialectInterface::Base<DialectInlinerInterface> {
+public:
+  DialectInlinerInterface(Dialect *dialect) : Base(dialect) {}
+
+  //===--------------------------------------------------------------------===//
+  // Analysis Hooks
+  //===--------------------------------------------------------------------===//
+
+  /// Returns true if the given operation 'callable', that implements the
+  /// 'CallableOpInterface', can be inlined into the position given call
+  /// operation 'call', that is registered to the current dialect and implements
+  /// the `CallOpInterface`. 'wouldBeCloned' is set to true if the region of the
+  /// given 'callable' is set to be cloned during the inlining process, or false
+  /// if the region is set to be moved in-place(i.e. no duplicates would be
+  /// created).
+  virtual bool isLegalToInline(Operation *call, Operation *callable,
+                               bool wouldBeCloned) const {
+    return false;
+  }
+
+  /// Returns true if the given region 'src' can be inlined into the region
+  /// 'dest' that is attached to an operation registered to the current dialect.
+  /// 'wouldBeCloned' is set to true if the given 'src' region is set to be
+  /// cloned during the inlining process, or false if the region is set to be
+  /// moved in-place(i.e. no duplicates would be created). 'valueMapping'
+  /// contains any remapped values from within the 'src' region. This can be
+  /// used to examine what values will replace entry arguments into the 'src'
+  /// region for example.
+  virtual bool isLegalToInline(Region *dest, Region *src, bool wouldBeCloned,
+                               IRMapping &valueMapping) const {
+    return false;
+  }
+
+  /// Returns true if the given operation 'op', that is registered to this
+  /// dialect, can be inlined into the given region, false otherwise.
+  /// 'wouldBeCloned' is set to true if the given 'op' is set to be cloned
+  /// during the inlining process, or false if the operation is set to be moved
+  /// in-place(i.e. no duplicates would be created). 'valueMapping' contains any
+  /// remapped values from within the 'src' region. This can be used to examine
+  /// what values may potentially replace the operands to 'op'.
+  virtual bool isLegalToInline(Operation *op, Region *dest, bool wouldBeCloned,
+                               IRMapping &valueMapping) const {
+    return false;
+  }
+
+  /// This hook is invoked on an operation that contains regions. It should
+  /// return true if the analyzer should recurse within the regions of this
+  /// operation when computing legality and cost, false otherwise. The default
+  /// implementation returns true.
+  virtual bool shouldAnalyzeRecursively(Operation *op) const { return true; }
+
+  //===--------------------------------------------------------------------===//
+  // Transformation Hooks
+  //===--------------------------------------------------------------------===//
+
+  /// Handle the given inlined terminator by replacing it with a new operation
+  /// as necessary. This overload is called when the inlined region has more
+  /// than one block. The 'newDest' block represents the new final branching
+  /// destination of blocks within this region, i.e. operations that release
+  /// control to the parent operation will likely now branch to this block.
+  /// Its block arguments correspond to any values that need to be replaced by
+  /// terminators within the inlined region.
+  virtual void handleTerminator(Operation *op, Block *newDest) const {
+    llvm_unreachable("must implement handleTerminator in the case of multiple "
+                     "inlined blocks");
+  }
+
+  /// Handle the given inlined terminator by replacing it with a new operation
+  /// as necessary. This overload is called when the inlined region only
+  /// contains one block. 'valuesToReplace' contains the previously returned
+  /// values of the call site before inlining. These values must be replaced by
+  /// this callback if they had any users (for example for traditional function
+  /// calls, these are directly replaced with the operands of the `return`
+  /// operation). The given 'op' will be removed by the caller, after this
+  /// function has been called.
+  virtual void handleTerminator(Operation *op,
+                                ValueRange valuesToReplace) const {
+    llvm_unreachable(
+        "must implement handleTerminator in the case of one inlined block");
+  }
+
+  /// Attempt to materialize a conversion for a type mismatch between a call
+  /// from this dialect, and a callable region. This method should generate an
+  /// operation that takes 'input' as the only operand, and produces a single
+  /// result of 'resultType'. If a conversion can not be generated, nullptr
+  /// should be returned. For example, this hook may be invoked in the following
+  /// scenarios:
+  ///   func @foo(i32) -> i32 { ... }
+  ///
+  ///   // Mismatched input operand
+  ///   ... = foo.call @foo(%input : i16) -> i32
+  ///
+  ///   // Mismatched result type.
+  ///   ... = foo.call @foo(%input : i32) -> i16
+  ///
+  /// NOTE: This hook may be invoked before the 'isLegal' checks above.
+  virtual Operation *materializeCallConversion(OpBuilder &builder, Value input,
+                                               Type resultType,
+                                               Location conversionLoc) const {
+    return nullptr;
+  }
+
+  /// Hook to transform the call arguments before using them to replace the
+  /// callee arguments. Returns a value of the same type or the `argument`
+  /// itself if nothing changed. The `argumentAttrs` dictionary is non-null even
+  /// if no attribute is present. The hook is called after converting the
+  /// callsite argument types using the materializeCallConversion callback, and
+  /// right before inlining the callee region. Any operations created using the
+  /// provided `builder` are inserted right before the inlined callee region. An
+  /// example use case is the insertion of copies for by value arguments.
+  virtual Value handleArgument(OpBuilder &builder, Operation *call,
+                               Operation *callable, Value argument,
+                               DictionaryAttr argumentAttrs) const {
+    return argument;
+  }
+
+  /// Hook to transform the callee results before using them to replace the call
+  /// results. Returns a value of the same type or the `result` itself if
+  /// nothing changed. The `resultAttrs` dictionary is non-null even if no
+  /// attribute is present. The hook is called right before handling
+  /// terminators, and obtains the callee result before converting its type
+  /// using the `materializeCallConversion` callback. Any operations created
+  /// using the provided `builder` are inserted right after the inlined callee
+  /// region. An example use case is the insertion of copies for by value
+  /// results. NOTE: This hook is invoked after inlining the `callable` region.
+  virtual Value handleResult(OpBuilder &builder, Operation *call,
+                             Operation *callable, Value result,
+                             DictionaryAttr resultAttrs) const {
+    return result;
+  }
+
+  /// Process a set of blocks that have been inlined for a call. This callback
+  /// is invoked before inlined terminator operations have been processed.
+  virtual void processInlinedCallBlocks(
+      Operation *call, iterator_range<Region::iterator> inlinedBlocks) const {}
+};
+
+/// This interface provides the hooks into the inlining interface.
+/// Note: this class automatically collects 'DialectInlinerInterface' objects
+/// registered to each dialect within the given context.
+class InlinerInterface
+    : public DialectInterfaceCollection<DialectInlinerInterface> {
+public:
+  using Base::Base;
+
+  /// Process a set of blocks that have been inlined. This callback is invoked
+  /// *before* inlined terminator operations have been processed.
+  virtual void
+  processInlinedBlocks(iterator_range<Region::iterator> inlinedBlocks) {}
+
+  /// These hooks mirror the hooks for the DialectInlinerInterface, with default
+  /// implementations that call the hook on the handler for the dialect 'op' is
+  /// registered to.
+
+  //===--------------------------------------------------------------------===//
+  // Analysis Hooks
+  //===--------------------------------------------------------------------===//
+
+  virtual bool isLegalToInline(Operation *call, Operation *callable,
+                               bool wouldBeCloned) const;
+  virtual bool isLegalToInline(Region *dest, Region *src, bool wouldBeCloned,
+                               IRMapping &valueMapping) const;
+  virtual bool isLegalToInline(Operation *op, Region *dest, bool wouldBeCloned,
+                               IRMapping &valueMapping) const;
+  virtual bool shouldAnalyzeRecursively(Operation *op) const;
+
+  //===--------------------------------------------------------------------===//
+  // Transformation Hooks
+  //===--------------------------------------------------------------------===//
+
+  virtual void handleTerminator(Operation *op, Block *newDest) const;
+  virtual void handleTerminator(Operation *op, ValueRange valuesToRepl) const;
+
+  virtual Value handleArgument(OpBuilder &builder, Operation *call,
+                               Operation *callable, Value argument,
+                               DictionaryAttr argumentAttrs) const;
+  virtual Value handleResult(OpBuilder &builder, Operation *call,
+                             Operation *callable, Value result,
+                             DictionaryAttr resultAttrs) const;
+
+  virtual void processInlinedCallBlocks(
+      Operation *call, iterator_range<Region::iterator> inlinedBlocks) const;
+};
+
+} // namespace mlir
+
+#endif // MLIR_INTERFACES_INLINERINTERFACE_H
diff --git a/mlir/include/mlir/Transforms/InliningUtils.h b/mlir/include/mlir/Transforms/InliningUtils.h
index 88fc033a6ab7be..16d12075fee9c4 100644
--- a/mlir/include/mlir/Transforms/InliningUtils.h
+++ b/mlir/include/mlir/Transforms/InliningUtils.h
@@ -13,217 +13,14 @@
 #ifndef MLIR_TRANSFORMS_INLININGUTILS_H
 #define MLIR_TRANSFORMS_INLININGUTILS_H
 
-#include "mlir/IR/BuiltinAttributes.h"
-#include "mlir/IR/DialectInterface.h"
-#include "mlir/IR/Location.h"
-#include "mlir/IR/Region.h"
 #include "mlir/IR/ValueRange.h"
+#include "mlir/Interfaces/InlinerInterface.h"
 #include <optional>
 
 namespace mlir {
 
-class Block;
-class IRMapping;
 class CallableOpInterface;
 class CallOpInterface;
-class OpBuilder;
-class Operation;
-class Region;
-class TypeRange;
-class Value;
-class ValueRange;
-
-//===----------------------------------------------------------------------===//
-// InlinerInterface
-//===----------------------------------------------------------------------===//
-
-/// This is the interface that must be implemented by the dialects of operations
-/// to be inlined. This interface should only handle the operations of the
-/// given dialect.
-class DialectInlinerInterface
-    : public DialectInterface::Base<DialectInlinerInterface> {
-public:
-  DialectInlinerInterface(Dialect *dialect) : Base(dialect) {}
-
-  //===--------------------------------------------------------------------===//
-  // Analysis Hooks
-  //===--------------------------------------------------------------------===//
-
-  /// Returns true if the given operation 'callable', that implements the
-  /// 'CallableOpInterface', can be inlined into the position given call
-  /// operation 'call', that is registered to the current dialect and implements
-  /// the `CallOpInterface`. 'wouldBeCloned' is set to true if the region of the
-  /// given 'callable' is set to be cloned during the inlining process, or false
-  /// if the region is set to be moved in-place(i.e. no duplicates would be
-  /// created).
-  virtual bool isLegalToInline(Operation *call, Operation *callable,
-                               bool wouldBeCloned) const {
-    return false;
-  }
-
-  /// Returns true if the given region 'src' can be inlined into the region
-  /// 'dest' that is attached to an operation registered to the current dialect.
-  /// 'wouldBeCloned' is set to true if the given 'src' region is set to be
-  /// cloned during the inlining process, or false if the region is set to be
-  /// moved in-place(i.e. no duplicates would be created). 'valueMapping'
-  /// contains any remapped values from within the 'src' region. This can be
-  /// used to examine what values will replace entry arguments into the 'src'
-  /// region for example.
-  virtual bool isLegalToInline(Region *dest, Region *src, bool wouldBeCloned,
-                               IRMapping &valueMapping) const {
-    return false;
-  }
-
-  /// Returns true if the given operation 'op', that is registered to this
-  /// dialect, can be inlined into the given region, false otherwise.
-  /// 'wouldBeCloned' is set to true if the given 'op' is set to be cloned
-  /// during the inlining process, or false if the operation is set to be moved
-  /// in-place(i.e. no duplicates would be created). 'valueMapping' contains any
-  /// remapped values from within the 'src' region. This can be used to examine
-  /// what values may potentially replace the operands to 'op'.
-  virtual bool isLegalToInline(Operation *op, Region *dest, bool wouldBeCloned,
-                               IRMapping &valueMapping) const {
-    return false;
-  }
-
-  /// This hook is invoked on an operation that contains regions. It should
-  /// return true if the analyzer should recurse within the regions of this
-  /// operation when computing legality and cost, false otherwise. The default
-  /// implementation returns true.
-  virtual bool shouldAnalyzeRecursively(Operation *op) const { return true; }
-
-  //===--------------------------------------------------------------------===//
-  // Transformation Hooks
-  //===--------------------------------------------------------------------===//
-
-  /// Handle the given inlined terminator by replacing it with a new operation
-  /// as necessary. This overload is called when the inlined region has more
-  /// than one block. The 'newDest' block represents the new final branching
-  /// destination of blocks within this region, i.e. operations that release
-  /// control to the parent operation will likely now branch to this block.
-  /// Its block arguments correspond to any values that need to be replaced by
-  /// terminators within the inlined region.
-  virtual void handleTerminator(Operation *op, Block *newDest) const {
-    llvm_unreachable("must implement handleTerminator in the case of multiple "
-                     "inlined blocks");
-  }
-
-  /// Handle the given inlined terminator by replacing it with a new operation
-  /// as necessary. This overload is called when the inlined region only
-  /// contains one block. 'valuesToReplace' contains the previously returned
-  /// values of the call site before inlining. These values must be replaced by
-  /// this callback if they had any users (for example for traditional function
-  /// calls, these are directly replaced with the operands of the `return`
-  /// operation). The given 'op' will be removed by the caller, after this
-  /// function has been called.
-  virtual void handleTerminator(Operation *op,
-                                ValueRange valuesToReplace) const {
-    llvm_unreachable(
-        "must implement handleTerminator in the case of one inlined block");
-  }
-
-  /// Attempt to materialize a conversion for a type mismatch between a call
-  /// from this dialect, and a callable region. This method should generate an
-  /// operation that takes 'input' as the only operand, and produces a single
-  /// result of 'resultType'. If a conversion can not be generated, nullptr
-  /// should be returned. For example, this hook may be invoked in the following
-  /// scenarios:
-  ///   func @foo(i32) -> i32 { ... }
-  ///
-  ///   // Mismatched input operand
-  ///   ... = foo.call @foo(%input : i16) -> i32
-  ///
-  ///   // Mismatched result type.
-  ///   ... = foo.call @foo(%input : i32) -> i16
-  ///
-  /// NOTE: This hook may be invoked before the 'isLegal' checks above.
-  virtual Operation *materializeCallConversion(OpBuilder &builder, Value input,
-                                               Type resultType,
-                                               Location conversionLoc) const {
-    return nullptr;
-  }
-
-  /// Hook to transform the call arguments before using them to replace the
-  /// callee arguments. Returns a value of the same type or the `argument`
-  /// itself if nothing changed. The `argumentAttrs` dictionary is non-null even
-  /// if no attribute is present. The hook is called after converting the
-  /// callsite argument types using the materializeCallConversion callback, and
-  /// right before inlining the callee region. Any operations created using the
-  /// provided `builder` are inserted right before the inlined callee region. An
-  /// example use case is the insertion of copies for by value arguments.
-  virtual Value handleArgument(OpBuilder &builder, Operation *call,
-                               Operation *callable, Value argument,
-                               DictionaryAttr argumentAttrs) const {
-    return argument;
-  }
-
-  /// Hook to transform the callee results before using them to replace the call
-  /// results. Returns a value of the same type or the `result` itself if
-  /// nothing changed. The `resultAttrs` dictionary is non-null even if no
-  /// attribute is present. The hook is called right before handling
-  /// terminators, and obtains the callee result before converting its type
-  /// using the `materializeCallConversion` callback. Any operations created
-  /// using the provided `builder` are inserted right after the inlined callee
-  /// region. An example use case is the insertion of copies for by value
-  /// results. NOTE: This hook is invoked after inlining the `callable` region.
-  virtual Value handleResult(OpBuilder &builder, Operation *call,
-                             Operation *callable, Value result,
-                             DictionaryAttr resultAttrs) const {
-    return result;
-  }
-
-  /// Process a set of blocks that have been inlined for a call. This callback
-  /// is invoked before inlined terminator operations have been processed.
-  virtual void processInlinedCallBlocks(
-      Operation *call, iterator_range<Region::iterator> inlinedBlocks) const {}
-};
-
-/// This interface provides the hooks into the inlining interface.
-/// Note: this class automatically collects 'DialectInlinerInterface' objects
-/// registered to each dialect within the given context.
-class InlinerInterface
-    : public DialectInterfaceCollection<DialectInlinerInterface> {
-public:
-  using Base::Base;
-...
[truncated]

@joker-eph
Copy link
Collaborator

Nit: your PR does not have a description. The title provides the "what" but the description should provide the context / the "why".

@chsigg
Copy link
Contributor Author

chsigg commented Mar 12, 2024

Nit: your PR does not have a description. The title provides the "what" but the description should provide the context / the "why".

Thanks, I added a description.

@joker-eph
Copy link
Collaborator

I don't see this layout as historical actually: to me the Interfaces folder is meant I believe to host the interfaces that aren't tied to specific transforms, instead they were meant for the common traits of the IR, basically what is needed during ODS generation or what describes the IR more generically than what's tied to a specific transform (like control-flow interface).

Looking at the list right now, there are a quite a few interfaces which don't belong here and are instead directly attached to a specific transform: to me they don't belong here.

@Mogball, @jpienaar, and @ftynse for the layering discussion here.

@chsigg
Copy link
Contributor Author

chsigg commented Mar 12, 2024

I see.

Another way to solve the circular dependency is to create a separate bazel target for just InliningUtils. After that, TransformUtils can be merged into Transforms without creating circular dependencies. Sorry for being bazel-centric here. I'm assuming that with CMake the layering is not as strict, but the layering is still not ideal. What is the relationship of MLIRTransform and MLIRTransformUtils?

Would it be better to move the InlinerInterface to a separate file and CMake library that still lives under Transforms/Utils?

@chsigg
Copy link
Contributor Author

chsigg commented Mar 12, 2024

to me the Interfaces folder is meant I believe to host the interfaces that aren't tied to specific transforms, instead they were meant for the common traits of the IR, basically what is needed during ODS generation or what describes the IR more generically than what's tied to a specific transform (like control-flow interface).

I see your point from the perspective of transform-specific interfaces. If you looked at it from a dialect perspective, which currently needs to depend on stuff from transform, the change suggested here might be easier to motivate.

That said, I'm happy to get guidance about what makes the most sense. I primarily see all the bazel targets that export the InliningUtils.h header, sticking out like sore thumbs.

@joker-eph
Copy link
Collaborator

CMake is indeed loose with header includes, and we're taking advantage of this for interfaces includes which are "header only" for implementing in a dialect.
For example that's why the builtin dialect can include these interfaces and not create a circular dependency.

I don't know how to model this with Bazel: would you create a "header only target" just for the header inclusion and another one for the link?

@Mogball
Copy link
Contributor

Mogball commented Mar 13, 2024

+1 to keeping transform specific interfaces in the TransformUtils library. The mlir/Interfaces directory, as Mehdi says, I think definitely reflects stronger and more first-class elements of the IR, like control-flow. I don't understand how this creates a circular dependency. What dialect depends on TransformUtils that Transform also depends on? You can always create an InlinerInterface library that lives under Transform/Utils.

@ftynse
Copy link
Member

ftynse commented Mar 13, 2024

A couple of counter-points, but I don't have a strong opinion.

  • Generally speaking, we have too many places where interfaces can live currently. Some interfaces are under IR, presumably because they were considered necessary for the IR definition (OpAsmInterface for printing/parsing, SymbolInterfaces for symbol tables) but some others are under Interface (ControlFlowInterfaces, DerivedAttributeInterface and SideEffectInterfaces all look as important as the things in IR). Some interfaces under Interface are specific to a transform (DestinationStyleInterface is clearly for bufferization and TilingInterface is for tiling) but this transform may not live in lib/Transform or there may be several variants of it. Some dialects have additional interfaces that create dialect dependencies, but may actually belong to a more generic Interface category. Do we really want to add yet another place where interfaces could live?
  • Transform/Utils is IMO a huge misnomer for interfaces. Utils are for additional things that may or may not be useful for somebody. Interfaces must be implemented for things to work.

@ftynse
Copy link
Member

ftynse commented Mar 13, 2024

I haven't checked how big are these libraries, but maybe we want to introduce a level of hierarchy under Transform, so there is lib/Transform/Inliner, lib/Transform/Tiling, lib/Transform/Loops` that can come with both the interfaces and the transforms that rely on those.

@chsigg
Copy link
Contributor Author

chsigg commented Mar 13, 2024

I don't know how to model this with Bazel: would you create a "header only target" just for the header inclusion and another one for the link?

You can do that, but then a user of your dialect needs to explicitly add the link target as a dependency. Ideally, the code is structured so targets are self-contained. This isn't hard in this particular case, we can move InliningUtils.h/cpp in a separate target and move on. I was just aiming for the cleaner approach, not expecting that moving the interface would be controversial.

I don't understand how this creates a circular dependency.

TransformUtils depends on various dialects via ViewLikeInterface and ArithUtils.

You can always create an InlinerInterface library that lives under Transform/Utils.

That's an option, yes. I just though the interface directory is the better place than Transform/Utils. The InliningInterface only depends on stuff from IR and nothing from Transforms.

lib/Transform/Inliner, lib/Transform/Tiling, lib/Transform/Loops that can come with both the interfaces and the transforms that rely on those.

That seems reasonable to me, but it's a bit of work and will likely break many downstream projects. I'm happy to do this if there is consensus that it's worth the trouble.

@Mogball
Copy link
Contributor

Mogball commented Mar 13, 2024

TransformUtils depends on various dialects via ViewLikeInterface and ArithUtils.

This seems like a huge problem. This is also affecting all downstream users.

@chsigg
Copy link
Contributor Author

chsigg commented Mar 13, 2024

TransformUtils depends on various dialects via ViewLikeInterface and ArithUtils.

Actually, there are much shorter paths, e.g. TransformUtils > Analysis > ArithDialect.

This seems like a huge problem. This is also affecting all downstream users.

I did find two places in TensorFlow where this seemed to be a problem, but no other ones. So hopefully this is still under control.

@joker-eph
Copy link
Collaborator

I don't know how to model this with Bazel: would you create a "header only target" just for the header inclusion and another one for the link?

You can do that, but then a user of your dialect needs to explicitly add the link target as a dependency.

No: they are really header-only dependency, you don't need the link target.

You need to link to register the interface implementation only IIRC.

CMake is lose with the header dependency, but has the same rules as Bazel for the link dependency (when we build with -DBUILD_SHARED_LIBS=ON)

@stellaraccident
Copy link
Contributor

I hear the "header only dep" argument but it still makes me quite uncomfortable as it represents project organization lines that should not be crossed: we have got to start thinking of some of these pieces as distinct projects.

@chsigg
Copy link
Contributor Author

chsigg commented Mar 14, 2024

No: they are really header-only dependency, you don't need the link target.
You need to link to register the interface implementation only IIRC.

Right, but dialect registration is part of the dialect target as well, and depending on just the header would make it not self-contained. Of course, most binary targets that depend on a few dialects will also depend on TransformUtils somehow. It does work the way it's set up right now, but the layering is not ideal and breaks e.g. managing dependencies automatically by tools.

@joker-eph
Copy link
Collaborator

Right, but dialect registration is part of the dialect target as well, and depending on just the header would make it not self-contained

I don't follow, can we look at one concrete use-case right now?
Which exact header and which exact link dependency is a problem?

@chsigg
Copy link
Contributor Author

chsigg commented Mar 14, 2024

Which exact header and which exact link dependency is a problem?

You are right, the DialectInlinerInterface that is used by the dialects is fully implemented in InliningUtils.h. So dialects could depend on a header-only target (let's call it InlinerInterface). The other declarations in InliningUtils.h would only be implemented in the TransformUtils target. This has the following problems (purely from a bazel perspective):

  • Either InliningUtils.h needs to be exported by both InlinerInterface and TransformUtils. Exporting headers multiple times is bad because it breaks modules and tools don't understand the difference between the two targets.
  • Alternatively, export InliningUtils only by InlinerInterface. This requires all (direct) users of the other declarations depend on both InlinerInterface and TransformUtils.

I fully understand that we do not want to restructure code to please the bazel build. We don't need to in this particular case (and I haven't looked into similar issues like the Bytecode headers yet), so if we agree that the DialectInlinerInterface should stay where it is, I can ...

  1. Fix this purely in the BUILD files by creating a separate target for InliningUtils.h/cpp.

The alternatives would be:

  1. This PR in its current form (moving the interfaces into the Interfaces directory).
  2. Do the same code split as in this PR, but move the files back to the Transforms directory.
  3. Move only DialectInlinerInterface to a separate (self-contained) header file, presumably in the Transforms directory.

If we could agree to one of these (or anything else), that would be great.

@joker-eph
Copy link
Collaborator

I looked at how we handled these cases before in Bazel, I believe the way to not confuse the tool and not have multiple targets exposing the header was to add them as srcs for the users: https://github.com/llvm/llvm-project/blob/main/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel#L330

@chsigg
Copy link
Contributor Author

chsigg commented Mar 15, 2024

Yes, consuming the header file under srcs is better than re-exporting it under hdrs. That's worse than an incomplete (just exporting the header) cc_library target, because it requires exporting the plain header file for downstream projects, see also the TensorFlow link above.

We don't need any of those crutches in this particular case though, we can make a target that exposes and implements the DialectInlinerInterface. I will go ahead with this and create a separate PR (approach 1. above). If you would like me to also move any interface definitions into separate files, please let me know.

chsigg added a commit that referenced this pull request Mar 16, 2024
Various (in-tree as well as downstream) targets currently depend on
`InliningUtils.h` to avoid circular dependencies. E.g. `TransformUtils`
depends on `ArithDialect`, so `ArithDialect` can't depend on
`TransformUtils` exporting `InliningUtils.h`. This change exposes that
header and it's implementation as a separate target. Having targets that
implement all the declared functions is the preferred approach for bazel
build graphs.

See also PR #84878, which moves the interface definitions to a separate
file in the `Interfaces` directory. This turned out to be controversial
and putting it in a different directory didn't seem to have any support
either. Instead, this PR only changes the bazel build without moving any
C++ code.
@chsigg chsigg closed this Mar 18, 2024
@chsigg chsigg deleted the piper_export_cl_614639900 branch March 26, 2024 14:02
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.

None yet

6 participants