Skip to content

Conversation

@matthias-springer
Copy link
Member

Fixes two minor issues in findOrBuildReplacementValue:

  • Remove a redundant mapping.map.
  • Map repl instead of value. We used to overwrite an existing mapping, which could introduce extra materializations.

Note: We generally do not want to overwrite mappings, but create a chain of mappings. There are still a few more places, where a mapping is overwritten. Once those are fixed, I will put an assertion into ConversionValueMapping::map.

Fixes two minor issues in `findOrBuildReplacementValue`:
* Remove a redundant `mapping.map`.
* Map `repl` instead of `value`. We used to overwrite an existing mapping, which may introduce extra materializations.

Note: We generally do not want to overwrite mappings, but create a chain of mappings. There are still a few more places, where a mapping is overwritten. Once those are fixed, I will put an assertion into `ConversionValueMapping::map`.
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Jan 4, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 4, 2025

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

Fixes two minor issues in findOrBuildReplacementValue:

  • Remove a redundant mapping.map.
  • Map repl instead of value. We used to overwrite an existing mapping, which could introduce extra materializations.

Note: We generally do not want to overwrite mappings, but create a chain of mappings. There are still a few more places, where a mapping is overwritten. Once those are fixed, I will put an assertion into ConversionValueMapping::map.


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

1 Files Affected:

  • (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+1-2)
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 6c3863e4c7f666..4904d3ce3f8635 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -1533,11 +1533,10 @@ Value ConversionPatternRewriterImpl::findOrBuildReplacementValue(
   Value castValue =
       buildUnresolvedMaterialization(MaterializationKind::Source,
                                      computeInsertPoint(repl), value.getLoc(),
-                                     /*valuesToMap=*/{value}, /*inputs=*/repl,
+                                     /*valuesToMap=*/repl, /*inputs=*/repl,
                                      /*outputType=*/value.getType(),
                                      /*originalType=*/Type(), converter)
           .front();
-  mapping.map(value, castValue);
   return castValue;
 }
 

@matthias-springer matthias-springer merged commit 5f7568a into main Jan 6, 2025
11 checks passed
@matthias-springer matthias-springer deleted the users/matthias-springer/fix_mapping_1 branch January 6, 2025 07:55
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