[mlir] fix dialect conversion with no type converter

[makslevental]

ConversionPatternRewriterImpl::remapValues has a bug when no type converter is used for a 1:N dialect conversion:

if (!currentTypeConverter) {
  // The current pattern does not have a type converter. I.e., it does not
  // distinguish between legal and illegal types. For each operand, simply
  // pass through the most recently mapped values.
  remapped.push_back(mapping.lookupOrDefault(operand));
  continue;
}

Emphasis on most recently mapped values.

But the implementation of mapping.lookupOrDefault in such a case does not coincide; it always recurses all the way back to the leaf values:

ValueVector
ConversionValueMapping::lookupOrDefault(...) const {
  ...
  do {
    ...
    if (TypeRange(ValueRange(current)) == desiredTypes)
      desiredValue = current;
    ....
    if (next != current) {
      // If at least one value was replaced, continue the lookup from there.
      current = std::move(next);
      continue;
    }
    ...
    if (it == mapping.end()) {
      // No mapping found: The lookup stops here.
      break;
    }
    current = it->second;
  } while (true);

  // If the desired values were found use them, otherwise default to the leaf values.
  return !desiredValue.empty() ? std::move(desiredValue) : std::move(current);
}

Note, that desiredTypes is empty and thus desiredValue.empty() is always true and thus std::move(current) is returned i.e., the leaf nodes.

The fix is to simply do what it says it should do i.e.:

  /// - If the desired type range is empty, simply return the most recently
  ///   mapped values.

[llvmbot]

@llvm/pr-subscribers-mlir-core

Author: Maksim Levental (makslevental)

Changes

ConversionPatternRewriterImpl::remapValues has a bug when no type converter is used for a 1:N dialect conversion:

if (!currentTypeConverter) {
  // The current pattern does not have a type converter. I.e., it does not
  // distinguish between legal and illegal types. For each operand, simply
  // pass through the most recently mapped values.
  remapped.push_back(mapping.lookupOrDefault(operand));
  continue;
}

Emphasis on most recently mapped values. But the implementation of mapping.lookupOrDefault in such a case always recurse all the way back to the leaf values:

ValueVector
ConversionValueMapping::lookupOrDefault(...) const {
  ...
  do {
    ...
    if (TypeRange(ValueRange(current)) == desiredTypes)
      desiredValue = current;
    ....
    if (next != current) {
      // If at least one value was replaced, continue the lookup from there.
      current = std::move(next);
      continue;
    }
    ...
    if (it == mapping.end()) {
      // No mapping found: The lookup stops here.
      break;
    }
    current = it->second;
  } while (true);

  // If the desired values were found use them, otherwise default to the leaf values.
  return !desiredValue.empty() ? std::move(desiredValue) : std::move(current);
}

Note, that desiredTypes is empty and thus desiredValue.empty() is always true and thus std::move(current) is returned i.e., the leaf nodes.

The fix is to condition continuing the while loop on !desiredTypes.empty() and otherwise break with current containing the first mapping lookup.

---

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

1 Files Affected:

• (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+1-1)

diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 403321d40d53c9..34e5d56bd95055 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -208,7 +208,7 @@ ConversionValueMapping::lookupOrDefault(Value from,
         next.push_back(v);
       }
     }
-    if (next != current) {
+    if (!desiredTypes.empty() && next != current) {
       // If at least one value was replaced, continue the lookup from there.
       current = std::move(next);
       continue;

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Maksim Levental (makslevental)

Changes

ConversionPatternRewriterImpl::remapValues has a bug when no type converter is used for a 1:N dialect conversion:

if (!currentTypeConverter) {
  // The current pattern does not have a type converter. I.e., it does not
  // distinguish between legal and illegal types. For each operand, simply
  // pass through the most recently mapped values.
  remapped.push_back(mapping.lookupOrDefault(operand));
  continue;
}

Emphasis on most recently mapped values. But the implementation of mapping.lookupOrDefault in such a case always recurse all the way back to the leaf values:

ValueVector
ConversionValueMapping::lookupOrDefault(...) const {
  ...
  do {
    ...
    if (TypeRange(ValueRange(current)) == desiredTypes)
      desiredValue = current;
    ....
    if (next != current) {
      // If at least one value was replaced, continue the lookup from there.
      current = std::move(next);
      continue;
    }
    ...
    if (it == mapping.end()) {
      // No mapping found: The lookup stops here.
      break;
    }
    current = it->second;
  } while (true);

  // If the desired values were found use them, otherwise default to the leaf values.
  return !desiredValue.empty() ? std::move(desiredValue) : std::move(current);
}

Note, that desiredTypes is empty and thus desiredValue.empty() is always true and thus std::move(current) is returned i.e., the leaf nodes.

The fix is to condition continuing the while loop on !desiredTypes.empty() and otherwise break with current containing the first mapping lookup.

---

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

1 Files Affected:

• (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+1-1)

diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 403321d40d53c9..34e5d56bd95055 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -208,7 +208,7 @@ ConversionValueMapping::lookupOrDefault(Value from,
         next.push_back(v);
       }
     }
-    if (next != current) {
+    if (!desiredTypes.empty() && next != current) {
       // If at least one value was replaced, continue the lookup from there.
       current = std::move(next);
       continue;