[flang-commits] [flang] [mlir] [mlir][Transforms] Support 1:N mappings in `ConversionValueMapping` (PR #116524)

[Matthias Springer via flang-commits]

Markus =?utf-8?q?Böck?= <markus.boeck02 at gmail.com>,Matthias Springer
 <mspringer at nvidia.com>,Matthias Springer <mspringer at nvidia.com>,Matthias
 Springer <mspringer at nvidia.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/116524 at github.com>


================
@@ -1478,34 +1497,12 @@ Value ConversionPatternRewriterImpl::findOrBuildReplacementValue(
   }
   Value castValue = buildUnresolvedMaterialization(
       MaterializationKind::Source, computeInsertPoint(repl), value.getLoc(),
----------------
matthias-springer wrote:

I thought about this a bit more. The problematic piece of code is `computeInsertionPoint(repl)` in `ConversionPatternRewriterImpl::findOrBuildReplacementValue`. I can't find a better way to implement this function.

- Alternative implementations like "always build materializations (no caching)" won't work here.
- Such alternatives would work for the other `computeInsertionPoint` call sites, but have certain downsides, as we discussed above.
- The main downside of the current `computeInsertionPoint(repl)` approach is increased code complexity (i.e., more lines of code to compute the insertion point). Performance should not be affected much because by far the most common case is when all values in `repl` are in the same block. This will hit the "fast path" (`chooseLaterInsertPointInBlock`), which does not traverse parent ops or compute dominance information.
- If we pay the price of increased code complexity anyway, we should also use this insertion point logic in all other places to avoid the downsides of the alternatives. (The complexity won't increase any further because of that.)

What do you think?


https://github.com/llvm/llvm-project/pull/116524