Skip to content

[mlir][Transforms][NFC] Dialect conversion: Remove "finalize" phase #116934

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

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

matthias-springer
Copy link
Member

@matthias-springer matthias-springer commented Nov 20, 2024

The dialect conversion driver has three phases:

  • Create IRRewrite objects as the IR is traversed.
  • Finalize IRRewrite objects. During this phase, source materializations for mismatching value types are created. (E.g., when Value is replaced with a Value of different type, but there is a user of the original value that was not modified because it is already legal.)
  • Commit IRRewrite objects. During this phase, all remaining IR modifications are materialized. In particular, SSA values are actually being replaced during this phase.

This commit removes the "finalize" phase. This simplifies the code base a bit and avoids one traversal over the IRRewrite stack. Source materializations are now built during the "commit" phase, right before an SSA value is being replaced.

This commit also removes the "inverse mapping" of the conversion value mapping, which was used to predict if an SSA value will be dead at the end of the conversion. This check is replaced with an approximate check that does not require an inverse mapping. (A false positive for v can occur if another value v2 is mapped to v and v2 turns out to be dead at the end of the conversion. This case is not expected to happen very often.) This reduces the complexity of the driver a bit and removes one potential source of bugs. (There have been bugs in the usage of the inverse mapping in the past.)

BlockTypeConversionRewrite no longer stores a pointer to the type converter. This pointer is now stored in ReplaceBlockArgRewrite.

This commit is in preparation of merging the 1:1 and 1:N dialect conversion driver. It simplifies the upcoming changes around the conversion value mapping. (API surface of the conversion value mapping is reduced.)

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Nov 20, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 20, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Matthias Springer (matthias-springer)

Changes

The dialect conversion driver has three phases:

  • Create IRRewrite objects as the IR is traversed.
  • Finalize IRRewrite objects. During this phase, source materializations for mismatching value types are created. (E.g., when Value is replaced with a Value of different type, but there is a user of the original value that was not modified because it is already legal.)
  • Commit IRRewrite objects. During this phase, all remaining IR modifications are materialized. In particular, SSA values are actually being replaced during this phase.

This commit removes the "finalize" phase. This simplifies the code base a bit and avoids one traversal over the IRRewrite stack. Source materializations are now built during the "commit" phase, right before an SSA value is being replaced.

This commit also removes the "inverse mapping" of the conversion value mapping, which was used to predict if an SSA value will be dead at the end of the conversion. This check is replaced with an approximate check that does not require an inverse mapping. This simplifies the upcoming changes around the conversion value mapping. (A false positive for v can occur if another value v2 is mapped to v and v2 turns out to be dead at the end of the conversion. This case is not expected to happen very often.)


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

1 Files Affected:

  • (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+72-112)
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 42fe5b925654a1..03d483f73f255e 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -75,6 +75,10 @@ namespace {
 /// This class wraps a IRMapping to provide recursive lookup
 /// functionality, i.e. we will traverse if the mapped value also has a mapping.
 struct ConversionValueMapping {
+  /// Return "true" if an SSA value is mapped to the given value. May return
+  /// false positives.
+  bool isMappedTo(Value value) const { return mappedTo.contains(value); }
+
   /// Lookup the most recently mapped value with the desired type in the
   /// mapping.
   ///
@@ -99,22 +103,18 @@ struct ConversionValueMapping {
         assert(it != oldVal && "inserting cyclic mapping");
     });
     mapping.map(oldVal, newVal);
+    mappedTo.insert(newVal);
   }
 
   /// Drop the last mapping for the given value.
   void erase(Value value) { mapping.erase(value); }
 
-  /// Returns the inverse raw value mapping (without recursive query support).
-  DenseMap<Value, SmallVector<Value>> getInverse() const {
-    DenseMap<Value, SmallVector<Value>> inverse;
-    for (auto &it : mapping.getValueMap())
-      inverse[it.second].push_back(it.first);
-    return inverse;
-  }
-
 private:
   /// Current value mappings.
   IRMapping mapping;
+
+  /// All SSA values that are mapped to. May contain false positives.
+  DenseSet<Value> mappedTo;
 };
 } // namespace
 
@@ -434,10 +434,9 @@ class MoveBlockRewrite : public BlockRewrite {
 class BlockTypeConversionRewrite : public BlockRewrite {
 public:
   BlockTypeConversionRewrite(ConversionPatternRewriterImpl &rewriterImpl,
-                             Block *block, Block *origBlock,
-                             const TypeConverter *converter)
+                             Block *block, Block *origBlock)
       : BlockRewrite(Kind::BlockTypeConversion, rewriterImpl, block),
-        origBlock(origBlock), converter(converter) {}
+        origBlock(origBlock) {}
 
   static bool classof(const IRRewrite *rewrite) {
     return rewrite->getKind() == Kind::BlockTypeConversion;
@@ -445,8 +444,6 @@ class BlockTypeConversionRewrite : public BlockRewrite {
 
   Block *getOrigBlock() const { return origBlock; }
 
-  const TypeConverter *getConverter() const { return converter; }
-
   void commit(RewriterBase &rewriter) override;
 
   void rollback() override;
@@ -454,9 +451,6 @@ class BlockTypeConversionRewrite : public BlockRewrite {
 private:
   /// The original block that was requested to have its signature converted.
   Block *origBlock;
-
-  /// The type converter used to convert the arguments.
-  const TypeConverter *converter;
 };
 
 /// Replacing a block argument. This rewrite is not immediately reflected in the
@@ -465,8 +459,10 @@ class BlockTypeConversionRewrite : public BlockRewrite {
 class ReplaceBlockArgRewrite : public BlockRewrite {
 public:
   ReplaceBlockArgRewrite(ConversionPatternRewriterImpl &rewriterImpl,
-                         Block *block, BlockArgument arg)
-      : BlockRewrite(Kind::ReplaceBlockArg, rewriterImpl, block), arg(arg) {}
+                         Block *block, BlockArgument arg,
+                         const TypeConverter *converter)
+      : BlockRewrite(Kind::ReplaceBlockArg, rewriterImpl, block), arg(arg),
+        converter(converter) {}
 
   static bool classof(const IRRewrite *rewrite) {
     return rewrite->getKind() == Kind::ReplaceBlockArg;
@@ -478,6 +474,9 @@ class ReplaceBlockArgRewrite : public BlockRewrite {
 
 private:
   BlockArgument arg;
+
+  /// The current type converter when the block argument was replaced.
+  const TypeConverter *converter;
 };
 
 /// An operation rewrite.
@@ -627,8 +626,6 @@ class ReplaceOperationRewrite : public OperationRewrite {
 
   void cleanup(RewriterBase &rewriter) override;
 
-  const TypeConverter *getConverter() const { return converter; }
-
 private:
   /// An optional type converter that can be used to materialize conversions
   /// between the new and old values if necessary.
@@ -825,6 +822,14 @@ struct ConversionPatternRewriterImpl : public RewriterBase::Listener {
                                  ValueRange replacements, Value originalValue,
                                  const TypeConverter *converter);
 
+  /// Find a replacement value for the given SSA value in the conversion value
+  /// mapping. The replacement value must have the same type as the given SSA
+  /// value. If there is no replacement value with the correct type, find the
+  /// latest replacement value (regardless of the type) and build a source
+  /// materialization.
+  Value findOrBuildReplacementValue(Value value,
+                                    const TypeConverter *converter);
+
   //===--------------------------------------------------------------------===//
   // Rewriter Notification Hooks
   //===--------------------------------------------------------------------===//
@@ -970,7 +975,7 @@ void BlockTypeConversionRewrite::rollback() {
 }
 
 void ReplaceBlockArgRewrite::commit(RewriterBase &rewriter) {
-  Value repl = rewriterImpl.mapping.lookupOrNull(arg, arg.getType());
+  Value repl = rewriterImpl.findOrBuildReplacementValue(arg, converter);
   if (!repl)
     return;
 
@@ -999,7 +1004,7 @@ void ReplaceOperationRewrite::commit(RewriterBase &rewriter) {
   // Compute replacement values.
   SmallVector<Value> replacements =
       llvm::map_to_vector(op->getResults(), [&](OpResult result) {
-        return rewriterImpl.mapping.lookupOrNull(result, result.getType());
+        return rewriterImpl.findOrBuildReplacementValue(result, converter);
       });
 
   // Notify the listener that the operation is about to be replaced.
@@ -1069,8 +1074,10 @@ void UnresolvedMaterializationRewrite::rollback() {
 void ConversionPatternRewriterImpl::applyRewrites() {
   // Commit all rewrites.
   IRRewriter rewriter(context, config.listener);
-  for (auto &rewrite : rewrites)
-    rewrite->commit(rewriter);
+  // Note: New rewrites may be added during the "commit" phase and the
+  // `rewrites` vector may reallocate.
+  for (size_t i = 0; i < rewrites.size(); ++i)
+    rewrites[i]->commit(rewriter);
 
   // Clean up all rewrites.
   for (auto &rewrite : rewrites)
@@ -1275,7 +1282,7 @@ Block *ConversionPatternRewriterImpl::applySignatureConversion(
           /*inputs=*/ValueRange(),
           /*outputType=*/origArgType, /*originalType=*/Type(), converter);
       mapping.map(origArg, repl);
-      appendRewrite<ReplaceBlockArgRewrite>(block, origArg);
+      appendRewrite<ReplaceBlockArgRewrite>(block, origArg, converter);
       continue;
     }
 
@@ -1285,7 +1292,7 @@ Block *ConversionPatternRewriterImpl::applySignatureConversion(
              "invalid to provide a replacement value when the argument isn't "
              "dropped");
       mapping.map(origArg, repl);
-      appendRewrite<ReplaceBlockArgRewrite>(block, origArg);
+      appendRewrite<ReplaceBlockArgRewrite>(block, origArg, converter);
       continue;
     }
 
@@ -1298,10 +1305,10 @@ Block *ConversionPatternRewriterImpl::applySignatureConversion(
     insertNTo1Materialization(
         OpBuilder::InsertPoint(newBlock, newBlock->begin()), origArg.getLoc(),
         /*replacements=*/replArgs, /*outputValue=*/origArg, converter);
-    appendRewrite<ReplaceBlockArgRewrite>(block, origArg);
+    appendRewrite<ReplaceBlockArgRewrite>(block, origArg, converter);
   }
 
-  appendRewrite<BlockTypeConversionRewrite>(newBlock, block, converter);
+  appendRewrite<BlockTypeConversionRewrite>(newBlock, block);
 
   // Erase the old block. (It is just unlinked for now and will be erased during
   // cleanup.)
@@ -1371,6 +1378,41 @@ void ConversionPatternRewriterImpl::insertNTo1Materialization(
   }
 }
 
+Value ConversionPatternRewriterImpl::findOrBuildReplacementValue(
+    Value value, const TypeConverter *converter) {
+  // Find a replacement value with the same type.
+  Value repl = mapping.lookupOrNull(value, value.getType());
+  if (repl)
+    return repl;
+
+  // Check if the value is dead. No replacement value is needed in that case.
+  // This is an approximate check that may have false negatives but does not
+  // require computing and traversing an inverse mapping. (We may end up
+  // building source materializations that are never used and that fold away.)
+  if (llvm::all_of(value.getUsers(),
+                   [&](Operation *op) { return replacedOps.contains(op); }) &&
+      !mapping.isMappedTo(value))
+    return Value();
+
+  // No replacement value was found. Get the latest replacement value
+  // (regardless of the type) and build a source materialization to the
+  // original type.
+  repl = mapping.lookupOrNull(value);
+  if (!repl) {
+    // No replacement value is registered in the mapping. This means that the
+    // value is dropped and no longer needed. (If the value were still needed,
+    // a source materialization producing a replacement value "out of thin air"
+    // would have already been created during `replaceOp` or
+    // `applySignatureConversion`.)
+    return Value();
+  }
+  Value castValue = buildUnresolvedMaterialization(
+      MaterializationKind::Source, computeInsertPoint(repl), value.getLoc(),
+      /*inputs=*/repl, /*outputType=*/value.getType(),
+      /*originalType=*/Type(), converter);
+  return castValue;
+}
+
 //===----------------------------------------------------------------------===//
 // Rewriter Notification Hooks
 
@@ -1597,7 +1639,8 @@ void ConversionPatternRewriter::replaceUsesOfBlockArgument(BlockArgument from,
                              << "'(in region of '" << parentOp->getName()
                              << "'(" << from.getOwner()->getParentOp() << ")\n";
   });
-  impl->appendRewrite<ReplaceBlockArgRewrite>(from.getOwner(), from);
+  impl->appendRewrite<ReplaceBlockArgRewrite>(from.getOwner(), from,
+                                              impl->currentTypeConverter);
   impl->mapping.map(impl->mapping.lookupOrDefault(from), to);
 }
 
@@ -2417,10 +2460,6 @@ struct OperationConverter {
   /// Converts an operation with the given rewriter.
   LogicalResult convert(ConversionPatternRewriter &rewriter, Operation *op);
 
-  /// This method is called after the conversion process to legalize any
-  /// remaining artifacts and complete the conversion.
-  void finalize(ConversionPatternRewriter &rewriter);
-
   /// Dialect conversion configuration.
   ConversionConfig config;
 
@@ -2541,11 +2580,6 @@ LogicalResult OperationConverter::convertOperations(ArrayRef<Operation *> ops) {
     if (failed(convert(rewriter, op)))
       return rewriterImpl.undoRewrites(), failure();
 
-  // Now that all of the operations have been converted, finalize the conversion
-  // process to ensure any lingering conversion artifacts are cleaned up and
-  // legalized.
-  finalize(rewriter);
-
   // After a successful conversion, apply rewrites.
   rewriterImpl.applyRewrites();
 
@@ -2579,80 +2613,6 @@ LogicalResult OperationConverter::convertOperations(ArrayRef<Operation *> ops) {
   return success();
 }
 
-/// Finds a user of the given value, or of any other value that the given value
-/// replaced, that was not replaced in the conversion process.
-static Operation *findLiveUserOfReplaced(
-    Value initialValue, ConversionPatternRewriterImpl &rewriterImpl,
-    const DenseMap<Value, SmallVector<Value>> &inverseMapping) {
-  SmallVector<Value> worklist = {initialValue};
-  while (!worklist.empty()) {
-    Value value = worklist.pop_back_val();
-
-    // Walk the users of this value to see if there are any live users that
-    // weren't replaced during conversion.
-    auto liveUserIt = llvm::find_if_not(value.getUsers(), [&](Operation *user) {
-      return rewriterImpl.isOpIgnored(user);
-    });
-    if (liveUserIt != value.user_end())
-      return *liveUserIt;
-    auto mapIt = inverseMapping.find(value);
-    if (mapIt != inverseMapping.end())
-      worklist.append(mapIt->second);
-  }
-  return nullptr;
-}
-
-/// Helper function that returns the replaced values and the type converter if
-/// the given rewrite object is an "operation replacement" or a "block type
-/// conversion" (which corresponds to a "block replacement"). Otherwise, return
-/// an empty ValueRange and a null type converter pointer.
-static std::pair<ValueRange, const TypeConverter *>
-getReplacedValues(IRRewrite *rewrite) {
-  if (auto *opRewrite = dyn_cast<ReplaceOperationRewrite>(rewrite))
-    return {opRewrite->getOperation()->getResults(), opRewrite->getConverter()};
-  if (auto *blockRewrite = dyn_cast<BlockTypeConversionRewrite>(rewrite))
-    return {blockRewrite->getOrigBlock()->getArguments(),
-            blockRewrite->getConverter()};
-  return {};
-}
-
-void OperationConverter::finalize(ConversionPatternRewriter &rewriter) {
-  ConversionPatternRewriterImpl &rewriterImpl = rewriter.getImpl();
-  DenseMap<Value, SmallVector<Value>> inverseMapping =
-      rewriterImpl.mapping.getInverse();
-
-  // Process requested value replacements.
-  for (unsigned i = 0, e = rewriterImpl.rewrites.size(); i < e; ++i) {
-    ValueRange replacedValues;
-    const TypeConverter *converter;
-    std::tie(replacedValues, converter) =
-        getReplacedValues(rewriterImpl.rewrites[i].get());
-    for (Value originalValue : replacedValues) {
-      // If the type of this value changed and the value is still live, we need
-      // to materialize a conversion.
-      if (rewriterImpl.mapping.lookupOrNull(originalValue,
-                                            originalValue.getType()))
-        continue;
-      Operation *liveUser =
-          findLiveUserOfReplaced(originalValue, rewriterImpl, inverseMapping);
-      if (!liveUser)
-        continue;
-
-      // Legalize this value replacement.
-      Value newValue = rewriterImpl.mapping.lookupOrNull(originalValue);
-      assert(newValue && "replacement value not found");
-      Value castValue = rewriterImpl.buildUnresolvedMaterialization(
-          MaterializationKind::Source, computeInsertPoint(newValue),
-          originalValue.getLoc(),
-          /*inputs=*/newValue, /*outputType=*/originalValue.getType(),
-          /*originalType=*/Type(), converter);
-      rewriterImpl.mapping.map(originalValue, castValue);
-      inverseMapping[castValue].push_back(originalValue);
-      llvm::erase(inverseMapping[newValue], originalValue);
-    }
-  }
-}
-
 //===----------------------------------------------------------------------===//
 // Reconcile Unrealized Casts
 //===----------------------------------------------------------------------===//

@matthias-springer matthias-springer changed the title [mlir][Transforms] Dialect conversion: Remove "finalize" phase [mlir][Transforms][NFC] Dialect conversion: Remove "finalize" phase Nov 20, 2024
@matthias-springer matthias-springer merged commit aa65473 into main Nov 21, 2024
11 checks passed
@matthias-springer matthias-springer deleted the users/matthias-springer/no_inverse_mapping branch November 21, 2024 01:26
@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 21, 2024

LLVM Buildbot has detected a new failure on builder mlir-rocm-mi200 running on mi200-buildbot while building mlir at step 6 "test-build-check-mlir-build-only-check-mlir".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/177/builds/8687

Here is the relevant piece of the build log for the reference
Step 6 (test-build-check-mlir-build-only-check-mlir) failure: test (failure)
******************** TEST 'MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir' FAILED ********************
Exit Code: 2

Command Output (stdout):
--
# RUN: at line 21
/vol/worker/mi200-buildbot/mlir-rocm-mi200/build/bin/mlir-opt /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir --sparsifier="enable-runtime-library=true" | /vol/worker/mi200-buildbot/mlir-rocm-mi200/build/bin/mlir-cpu-runner -e main -entry-point-result=void -shared-libs=/vol/worker/mi200-buildbot/mlir-rocm-mi200/build/lib/libmlir_c_runner_utils.so,/vol/worker/mi200-buildbot/mlir-rocm-mi200/build/lib/libmlir_runner_utils.so | /vol/worker/mi200-buildbot/mlir-rocm-mi200/build/bin/FileCheck /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir
# executed command: /vol/worker/mi200-buildbot/mlir-rocm-mi200/build/bin/mlir-opt /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir --sparsifier=enable-runtime-library=true
# .---command stderr------------
# | /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir:244:11: error: null operand found
# |     %v8 = vector.transfer_read %collapse8[%c0, %c0], %df: tensor<?x?xf64>, vector<6x10xf64>
# |           ^
# | /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir:244:11: note: see current operation: %949 = "arith.index_cast"(<<NULL VALUE>>) : (<<NULL TYPE>>) -> i32
# `-----------------------------
# error: command failed with exit status: 1
# executed command: /vol/worker/mi200-buildbot/mlir-rocm-mi200/build/bin/mlir-cpu-runner -e main -entry-point-result=void -shared-libs=/vol/worker/mi200-buildbot/mlir-rocm-mi200/build/lib/libmlir_c_runner_utils.so,/vol/worker/mi200-buildbot/mlir-rocm-mi200/build/lib/libmlir_runner_utils.so
# .---command stderr------------
# | Error: entry point not found
# `-----------------------------
# error: command failed with exit status: 1
# executed command: /vol/worker/mi200-buildbot/mlir-rocm-mi200/build/bin/FileCheck /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir
# .---command stderr------------
# | FileCheck error: '<stdin>' is empty.
# | FileCheck command line:  /vol/worker/mi200-buildbot/mlir-rocm-mi200/build/bin/FileCheck /vol/worker/mi200-buildbot/mlir-rocm-mi200/llvm-project/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir
# `-----------------------------
# error: command failed with exit status: 2

--

********************


@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 21, 2024

LLVM Buildbot has detected a new failure on builder mlir-nvidia-gcc7 running on mlir-nvidia while building mlir at step 6 "test-build-check-mlir-build-only-check-mlir".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/6654

Here is the relevant piece of the build log for the reference
Step 6 (test-build-check-mlir-build-only-check-mlir) failure: test (failure)
******************** TEST 'MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_conv_1d_nwc_wcf.mlir' FAILED ********************
Exit Code: 2

Command Output (stdout):
--
# RUN: at line 21
/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/bin/mlir-opt /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conv_1d_nwc_wcf.mlir --sparsifier="enable-runtime-library=true" | /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/bin/mlir-cpu-runner -e main -entry-point-result=void -shared-libs=/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/lib/libmlir_c_runner_utils.so,/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/lib/libmlir_runner_utils.so | /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/bin/FileCheck /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conv_1d_nwc_wcf.mlir
# executed command: /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/bin/mlir-opt /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conv_1d_nwc_wcf.mlir --sparsifier=enable-runtime-library=true
# .---command stderr------------
# | /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conv_1d_nwc_wcf.mlir:111:14: error: null operand found
# |   %dense_v = vector.transfer_read %dense_ret[%c0, %c0, %c0], %zero
# |              ^
# | /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conv_1d_nwc_wcf.mlir:111:14: note: see current operation: %315 = "arith.index_cast"(<<NULL VALUE>>) : (<<NULL TYPE>>) -> i32
# `-----------------------------
# error: command failed with exit status: 1
# executed command: /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/bin/mlir-cpu-runner -e main -entry-point-result=void -shared-libs=/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/lib/libmlir_c_runner_utils.so,/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/lib/libmlir_runner_utils.so
# .---command stderr------------
# | Error: entry point not found
# `-----------------------------
# error: command failed with exit status: 1
# executed command: /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/bin/FileCheck /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conv_1d_nwc_wcf.mlir
# .---command stderr------------
# | FileCheck error: '<stdin>' is empty.
# | FileCheck command line:  /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.obj/bin/FileCheck /vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_conv_1d_nwc_wcf.mlir
# `-----------------------------
# error: command failed with exit status: 2

--

********************


@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 21, 2024

LLVM Buildbot has detected a new failure on builder mlir-nvidia running on mlir-nvidia while building mlir at step 6 "test-build-check-mlir-build-only-check-mlir".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/6727

Here is the relevant piece of the build log for the reference
Step 6 (test-build-check-mlir-build-only-check-mlir) failure: test (failure)
******************** TEST 'MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir' FAILED ********************
Exit Code: 2

Command Output (stdout):
--
# RUN: at line 21
/vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/bin/mlir-opt /vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir --sparsifier="enable-runtime-library=true" | /vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/bin/mlir-cpu-runner -e main -entry-point-result=void -shared-libs=/vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/lib/libmlir_c_runner_utils.so,/vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/lib/libmlir_runner_utils.so | /vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/bin/FileCheck /vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir
# executed command: /vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/bin/mlir-opt /vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir --sparsifier=enable-runtime-library=true
# .---command stderr------------
# | /vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir:244:11: error: null operand found
# |     %v8 = vector.transfer_read %collapse8[%c0, %c0], %df: tensor<?x?xf64>, vector<6x10xf64>
# |           ^
# | /vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir:244:11: note: see current operation: %949 = "arith.index_cast"(<<NULL VALUE>>) : (<<NULL TYPE>>) -> i32
# `-----------------------------
# error: command failed with exit status: 1
# executed command: /vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/bin/mlir-cpu-runner -e main -entry-point-result=void -shared-libs=/vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/lib/libmlir_c_runner_utils.so,/vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/lib/libmlir_runner_utils.so
# .---command stderr------------
# | Error: entry point not found
# `-----------------------------
# error: command failed with exit status: 1
# executed command: /vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/bin/FileCheck /vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir
# .---command stderr------------
# | FileCheck error: '<stdin>' is empty.
# | FileCheck command line:  /vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/bin/FileCheck /vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_collapse_shape.mlir
# `-----------------------------
# error: command failed with exit status: 2

--

********************


matthias-springer added a commit that referenced this pull request Nov 21, 2024
matthias-springer added a commit that referenced this pull request Nov 21, 2024
…116934)

The dialect conversion driver has three phases:
- **Create** `IRRewrite` objects as the IR is traversed.
- **Finalize** `IRRewrite` objects. During this phase, source
materializations for mismatching value types are created. (E.g., when
`Value` is replaced with a `Value` of different type, but there is a
user of the original value that was not modified because it is already
legal.)
- **Commit** `IRRewrite` objects. During this phase, all remaining IR
modifications are materialized. In particular, SSA values are actually
being replaced during this phase.

This commit removes the "finalize" phase. This simplifies the code base
a bit and avoids one traversal over the `IRRewrite` stack. Source
materializations are now built during the "commit" phase, right before
an SSA value is being replaced.

This commit also removes the "inverse mapping" of the conversion value
mapping, which was used to predict if an SSA value will be dead at the
end of the conversion. This check is replaced with an approximate check
that does not require an inverse mapping. (A false positive for `v` can
occur if another value `v2` is mapped to `v` and `v2` turns out to be
dead at the end of the conversion. This case is not expected to happen
very often.) This reduces the complexity of the driver a bit and removes
one potential source of bugs. (There have been bugs in the usage of the
inverse mapping in the past.)

`BlockTypeConversionRewrite` no longer stores a pointer to the type
converter. This pointer is now stored in `ReplaceBlockArgRewrite`.

This commit is in preparation of merging the 1:1 and 1:N dialect
conversion driver. It simplifies the upcoming changes around the
conversion value mapping. (API surface of the conversion value mapping
is reduced.)
matthias-springer added a commit that referenced this pull request Nov 22, 2024
…116934)

The dialect conversion driver has three phases:
- **Create** `IRRewrite` objects as the IR is traversed.
- **Finalize** `IRRewrite` objects. During this phase, source
materializations for mismatching value types are created. (E.g., when
`Value` is replaced with a `Value` of different type, but there is a
user of the original value that was not modified because it is already
legal.)
- **Commit** `IRRewrite` objects. During this phase, all remaining IR
modifications are materialized. In particular, SSA values are actually
being replaced during this phase.

This commit removes the "finalize" phase. This simplifies the code base
a bit and avoids one traversal over the `IRRewrite` stack. Source
materializations are now built during the "commit" phase, right before
an SSA value is being replaced.

This commit also removes the "inverse mapping" of the conversion value
mapping, which was used to predict if an SSA value will be dead at the
end of the conversion. This check is replaced with an approximate check
that does not require an inverse mapping. (A false positive for `v` can
occur if another value `v2` is mapped to `v` and `v2` turns out to be
dead at the end of the conversion. This case is not expected to happen
very often.) This reduces the complexity of the driver a bit and removes
one potential source of bugs. (There have been bugs in the usage of the
inverse mapping in the past.)

`BlockTypeConversionRewrite` no longer stores a pointer to the type
converter. This pointer is now stored in `ReplaceBlockArgRewrite`.

This commit is in preparation of merging the 1:1 and 1:N dialect
conversion driver. It simplifies the upcoming changes around the
conversion value mapping. (API surface of the conversion value mapping
is reduced.)
matthias-springer added a commit that referenced this pull request Nov 22, 2024
…117097)

This is a re-upload of #116934, which was reverted.

The dialect conversion driver has three phases:
- **Create** `IRRewrite` objects as the IR is traversed.
- **Finalize** `IRRewrite` objects. During this phase, source
materializations for mismatching value types are created. (E.g., when
`Value` is replaced with a `Value` of different type, but there is a
user of the original value that was not modified because it is already
legal.)
- **Commit** `IRRewrite` objects. During this phase, all remaining IR
modifications are materialized. In particular, SSA values are actually
being replaced during this phase.

This commit removes the "finalize" phase. This simplifies the code base
a bit and avoids one traversal over the `IRRewrite` stack. Source
materializations are now built during the "commit" phase, right before
an SSA value is being replaced.

This commit also removes the "inverse mapping" of the conversion value
mapping, which was used to predict if an SSA value will be dead at the
end of the conversion. This check is replaced with an approximate check
that does not require an inverse mapping. (A false positive for `v` can
occur if another value `v2` is mapped to `v` and `v2` turns out to be
dead at the end of the conversion. This case is not expected to happen
very often.) This reduces the complexity of the driver a bit and removes
one potential source of bugs. (There have been bugs in the usage of the
inverse mapping in the past.)

`BlockTypeConversionRewrite` no longer stores a pointer to the type
converter. This pointer is now stored in `ReplaceBlockArgRewrite`.

This commit is in preparation of merging the 1:1 and 1:N dialect
conversion driver. It simplifies the upcoming changes around the
conversion value mapping. (API surface of the conversion value mapping
is reduced.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants