[mlir][Transforms] Dialect Conversion: Do not overwrite mapping

[matthias-springer]

This commit adds an assertion to check that the ConversionValueMapping is not silently overwritten. Doing so would indicate a bug in the conversion driver or incorrect API usage.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

This commit adds an assertion to check that the ConversionValueMapping is not silently overwritten. Doing so would indicate a bug in the conversion driver or incorrect API usage.

---

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

1 Files Affected:

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

diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 7de26d7cfa84d..4cf9b846da224 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -144,6 +144,8 @@ struct ConversionValueMapping {
   template <typename OldVal, typename NewVal>
   std::enable_if_t<IsValueVector<OldVal>::value && IsValueVector<NewVal>::value>
   map(OldVal &&oldVal, NewVal &&newVal) {
+    assert(mapping.find(oldVal) == mapping.end() &&
+           "attempting to overwrite mapping");
     LLVM_DEBUG({
       ValueVector next(newVal);
       while (true) {

[zero9178]

LGTM with minor nit and assuming CI failure gets fixed (seems real)

[zero9178]

It seems as if this assert will be user facing (eg if users accidently replace a value more than once?). Could we rephrase the assert message such that it makes more sense from an outside perspective, maybe something like

Cannot replace value more than once
or similar

[matthias-springer]

The assertion fails in a test case. The problem is that:

1. A materialization is created for a block argument. Materializations are stored in the mapping.
2. The block is later converted, at which point the mapping is overwritten.

There seems to be a general problem with the way we deal with materializations. I'm not sure how to fix that. Closing this PR for now.