[Mlir-commits] [mlir] 1c9c2c9 - [mlir] Remove the default isDynamicallyLegal hook

[Benjamin Kramer]

Author: Benjamin Kramer
Date: 2021-07-29T11:00:57+02:00
New Revision: 1c9c2c91d4d4b62d99ea1472de4d01eb7d7e6ee0

URL: https://github.com/llvm/llvm-project/commit/1c9c2c91d4d4b62d99ea1472de4d01eb7d7e6ee0
DIFF: https://github.com/llvm/llvm-project/commit/1c9c2c91d4d4b62d99ea1472de4d01eb7d7e6ee0.diff

LOG: [mlir] Remove the default isDynamicallyLegal hook

This is redundant with the callback variant and untested. Also remove
the callback-less methods for adding a dynamically legal op, as they
are no longer useful.

Differential Revision: https://reviews.llvm.org/D106786

Added: 
    

Modified: 
    mlir/docs/DialectConversion.md
    mlir/include/mlir/Transforms/DialectConversion.h
    mlir/lib/Transforms/Utils/DialectConversion.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/docs/DialectConversion.md b/mlir/docs/DialectConversion.md
index b74e6656c57d8..40cba64232f6f 100644
--- a/mlir/docs/DialectConversion.md
+++ b/mlir/docs/DialectConversion.md
@@ -67,9 +67,6 @@ legality actions below:
     -   This action signals that only some instances of a given operation are
         legal. This allows for defining fine-tune constraints, e.g. saying that
         `addi` is only legal when operating on 32-bit integers.
-    -   If a specific handler is not provided when setting the action, the
-        target must override the `isDynamicallyLegal` hook provided by
-        `ConversionTarget`.
 
 *   Illegal
 
@@ -97,10 +94,7 @@ struct MyTarget : public ConversionTarget {
 
     /// Mark all operations within Affine dialect have dynamic legality
     /// constraints.
-    addDynamicallyLegalDialect<AffineDialect>();
-
-    /// Mark `std.return` as dynamically legal.
-    addDynamicallyLegalOp<ReturnOp>();
+    addDynamicallyLegalDialect<AffineDialect>([](Operation *op) { ... });
 
     /// Mark `std.return` as dynamically legal, but provide a specific legality
     /// callback.
@@ -108,7 +102,6 @@ struct MyTarget : public ConversionTarget {
 
     /// Treat unknown operations, i.e. those without a legalization action
     /// directly set, as dynamically legal.
-    markUnknownOpDynamicallyLegal();
     markUnknownOpDynamicallyLegal([](Operation *op) { ... });
 
     //--------------------------------------------------------------------------

diff  --git a/mlir/include/mlir/Transforms/DialectConversion.h b/mlir/include/mlir/Transforms/DialectConversion.h
index e490ec14f3581..c9e9f67483996 100644
--- a/mlir/include/mlir/Transforms/DialectConversion.h
+++ b/mlir/include/mlir/Transforms/DialectConversion.h
@@ -622,7 +622,6 @@ class ConversionTarget {
   using DynamicLegalityCallbackFn = std::function<bool(Operation *)>;
 
   ConversionTarget(MLIRContext &ctx) : ctx(ctx) {}
-  virtual ~ConversionTarget() = default;
 
   //===--------------------------------------------------------------------===//
   // Legality Registration
@@ -646,18 +645,6 @@ class ConversionTarget {
     addLegalOp<OpT2, OpTs...>();
   }
 
-  /// Register the given operation as dynamically legal, i.e. requiring custom
-  /// handling by the target via 'isDynamicallyLegal'.
-  template <typename OpT>
-  void addDynamicallyLegalOp() {
-    setOpAction<OpT>(LegalizationAction::Dynamic);
-  }
-  template <typename OpT, typename OpT2, typename... OpTs>
-  void addDynamicallyLegalOp() {
-    addDynamicallyLegalOp<OpT>();
-    addDynamicallyLegalOp<OpT2, OpTs...>();
-  }
-
   /// Register the given operation as dynamically legal and set the dynamic
   /// legalization callback to the one provided.
   template <typename OpT>
@@ -731,31 +718,26 @@ class ConversionTarget {
   }
 
   /// Register the operations of the given dialects as dynamically legal, i.e.
-  /// requiring custom handling by the target via 'isDynamicallyLegal'.
+  /// requiring custom handling by the callback.
   template <typename... Names>
-  void addDynamicallyLegalDialect(StringRef name, Names... names) {
+  void addDynamicallyLegalDialect(DynamicLegalityCallbackFn callback,
+                                  StringRef name, Names... names) {
     SmallVector<StringRef, 2> dialectNames({name, names...});
     setDialectAction(dialectNames, LegalizationAction::Dynamic);
+    setLegalityCallback(dialectNames, std::move(callback));
   }
   template <typename... Args>
-  void addDynamicallyLegalDialect(DynamicLegalityCallbackFn callback = {}) {
-    SmallVector<StringRef, 2> dialectNames({Args::getDialectNamespace()...});
-    setDialectAction(dialectNames, LegalizationAction::Dynamic);
-    if (callback)
-      setLegalityCallback(dialectNames, callback);
+  void addDynamicallyLegalDialect(DynamicLegalityCallbackFn callback) {
+    addDynamicallyLegalDialect(std::move(callback),
+                               Args::getDialectNamespace()...);
   }
 
   /// Register unknown operations as dynamically legal. For operations(and
   /// dialects) that do not have a set legalization action, treat them as
-  /// dynamically legal and invoke the given callback if valid or
-  /// 'isDynamicallyLegal'.
+  /// dynamically legal and invoke the given callback.
   void markUnknownOpDynamicallyLegal(const DynamicLegalityCallbackFn &fn) {
     setLegalityCallback(fn);
   }
-  void markUnknownOpDynamicallyLegal() {
-    setLegalityCallback(
-        [this](Operation *op) { return isDynamicallyLegal(op); });
-  }
 
   /// Register the operations of the given dialects as illegal, i.e.
   /// operations of this dialect are not supported by the target.
@@ -782,14 +764,6 @@ class ConversionTarget {
   /// legal, None is returned.
   Optional<LegalOpDetails> isLegal(Operation *op) const;
 
-protected:
-  /// Runs a custom legalization query for the given operation. This should
-  /// return true if the given operation is legal, otherwise false.
-  virtual bool isDynamicallyLegal(Operation *op) const {
-    llvm_unreachable(
-        "targets with custom legalization must override 'isDynamicallyLegal'");
-  }
-
 private:
   /// Set the dynamic legality callback for the given operation.
   void setLegalityCallback(OperationName name,

diff  --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 5fbd9dd9db60a..6aa42f64059fd 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -2700,10 +2700,9 @@ auto ConversionTarget::isLegal(Operation *op) const
 
   // Returns true if this operation instance is known to be legal.
   auto isOpLegal = [&] {
-    // Handle dynamic legality either with the provided legality function, or
-    // the default hook on the derived instance.
+    // Handle dynamic legality either with the provided legality function.
     if (info->action == LegalizationAction::Dynamic)
-      return info->legalityFn ? info->legalityFn(op) : isDynamicallyLegal(op);
+      return info->legalityFn(op);
 
     // Otherwise, the operation is only legal if it was marked 'Legal'.
     return info->action == LegalizationAction::Legal;