[Mlir-commits] [mlir] 9ca67d7 - Revert "[mlir] Lookup the latest value with a legal type when remapping values."

[Alexander Belyaev]

Author: Alexander Belyaev
Date: 2020-12-16T23:09:04+01:00
New Revision: 9ca67d7f4467a5dab4287e7c431f9daf313ca38a

URL: https://github.com/llvm/llvm-project/commit/9ca67d7f4467a5dab4287e7c431f9daf313ca38a
DIFF: https://github.com/llvm/llvm-project/commit/9ca67d7f4467a5dab4287e7c431f9daf313ca38a.diff

LOG: Revert "[mlir] Lookup the latest value with a legal type when remapping values."

This reverts commit f8184d4c44dff1fab13122221f0c23ab50936647.

Added: 
    

Modified: 
    mlir/lib/Transforms/Utils/DialectConversion.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 61371b2c08b6..0a1a6b712ff2 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -105,15 +105,10 @@ namespace {
 /// functionality, i.e. we will traverse if the mapped value also has a mapping.
 struct ConversionValueMapping {
   /// Lookup a mapped value within the map. If a mapping for the provided value
-  /// does not exist then return the provided value.
-  Value lookupOrDefault(Value from) const;
-
-  /// Lookup the latest legal value within the map. If a mapping for the
-  /// provided value does not exist then return the provided value. If
-  /// `converter` is non-null, returns the most recently mapped value with the
-  /// legal type. If an operand of that type does not exist, defaults to normal
-  /// behavior.
-  Value lookupLatestLegal(Value from, TypeConverter *converter) const;
+  /// does not exist then return the provided value. If `desiredType` is
+  /// non-null, returns the most recently mapped value with that type. If an
+  /// operand of that type does not exist, defaults to normal behavior.
+  Value lookupOrDefault(Value from, Type desiredType = nullptr) const;
 
   /// Lookup a mapped value within the map, or return null if a mapping does not
   /// exist. If a mapping exists, this follows the same behavior of
@@ -132,24 +127,22 @@ struct ConversionValueMapping {
 };
 } // end anonymous namespace
 
-Value ConversionValueMapping::lookupOrDefault(Value from) const {
-  // If this value had a valid mapping, unmap that value as well in the case
-  // that it was also replaced.
-  while (auto mappedValue = mapping.lookupOrNull(from))
-    from = mappedValue;
-  return from;
-}
-
-Value ConversionValueMapping::lookupLatestLegal(
-    Value from, TypeConverter *converter) const {
-  if (!converter)
-    return lookupOrDefault(from);
+Value ConversionValueMapping::lookupOrDefault(Value from,
+                                              Type desiredType) const {
+  // If there was no desired type, simply find the leaf value.
+  if (!desiredType) {
+    // If this value had a valid mapping, unmap that value as well in the case
+    // that it was also replaced.
+    while (auto mappedValue = mapping.lookupOrNull(from))
+      from = mappedValue;
+    return from;
+  }
 
-  // Otherwise, try to find the deepest value that has the legal type.
-  Value legalValue;
+  // Otherwise, try to find the deepest value that has the desired type.
+  Value desiredValue;
   do {
-    if (converter->isLegal(from.getType()))
-      legalValue = from;
+    if (from.getType() == desiredType)
+      desiredValue = from;
 
     Value mappedValue = mapping.lookupOrNull(from);
     if (!mappedValue)
@@ -158,7 +151,7 @@ Value ConversionValueMapping::lookupLatestLegal(
   } while (true);
 
   // If the desired value was found use it, otherwise default to the leaf value.
-  return legalValue ? legalValue : from;
+  return desiredValue ? desiredValue : from;
 }
 
 Value ConversionValueMapping::lookupOrNull(Value from) const {
@@ -1046,41 +1039,22 @@ LogicalResult ConversionPatternRewriterImpl::remapValues(
     Value operand = it.value();
     Type origType = operand.getType();
 
-    Value newOperand = mapping.lookupLatestLegal(operand, converter);
-
-    // Handle the case where the conversion was 1->1 and the new operand type
-    // isn't legal.
-    Type newOperandType = newOperand.getType();
+    // If a converter was provided, get the desired legal types for this
+    // operand.
+    Type desiredType;
     if (converter) {
-      if (!converter->isLegal(newOperandType)) {
-        legalTypes.clear();
-
-        // If there is no legal conversion, fail to match this pattern.
-        if (failed(converter->convertType(origType, legalTypes))) {
-          return notifyMatchFailure(loc, [=](Diagnostic &diag) {
-            diag << "unable to convert type for operand #" << it.index()
-                 << ", type was " << origType;
-          });
-        }
-        // TODO: There currently isn't any mechanism to do 1->N type conversion
-        // via the PatternRewriter replacement API, so for now we just ignore
-        // it.
-        if (legalTypes.size() != 1) {
-          remapped.push_back(newOperand);
-          continue;
-        }
-        Type desiredType = legalTypes.front();
-        newOperand = converter->materializeTargetConversion(
-            rewriter, loc, desiredType, newOperand);
-        if (!newOperand) {
-          return notifyMatchFailure(loc, [=](Diagnostic &diag) {
-            diag << "unable to materialize a conversion for "
-                    "operand #"
-                 << it.index() << ", from " << newOperandType << " to "
-                 << desiredType;
-          });
-        }
+      // If there is no legal conversion, fail to match this pattern.
+      legalTypes.clear();
+      if (failed(converter->convertType(origType, legalTypes))) {
+        return notifyMatchFailure(loc, [=](Diagnostic &diag) {
+          diag << "unable to convert type for operand #" << it.index()
+               << ", type was " << origType;
+        });
       }
+      // TODO: There currently isn't any mechanism to do 1->N type conversion
+      // via the PatternRewriter replacement API, so for now we just ignore it.
+      if (legalTypes.size() == 1)
+        desiredType = legalTypes.front();
     } else {
       // TODO: What we should do here is just set `desiredType` to `origType`
       // and then handle the necessary type conversions after the conversion
@@ -1088,7 +1062,24 @@ LogicalResult ConversionPatternRewriterImpl::remapValues(
       // receiving the new operands even if the types change, so we keep the
       // original behavior here for now until all of the patterns relying on
       // this get updated.
+    }
+    Value newOperand = mapping.lookupOrDefault(operand, desiredType);
+
+    // Handle the case where the conversion was 1->1 and the new operand type
+    // isn't legal.
+    Type newOperandType = newOperand.getType();
+    if (converter && desiredType && newOperandType != desiredType) {
       // Attempt to materialize a conversion for this new value.
+      newOperand = converter->materializeTargetConversion(
+          rewriter, loc, desiredType, newOperand);
+      if (!newOperand) {
+        return notifyMatchFailure(loc, [=](Diagnostic &diag) {
+          diag << "unable to materialize a conversion for "
+                  "operand #"
+               << it.index() << ", from " << newOperandType << " to "
+               << desiredType;
+        });
+      }
     }
     remapped.push_back(newOperand);
   }