[Mlir-commits] [mlir] [mlir][Transforms] Dialect Conversion: Simplify block conversion API (PR #94866)

[Matthias Springer]

================
@@ -661,42 +662,38 @@ class ConversionPatternRewriter final : public PatternRewriter {
 public:
   ~ConversionPatternRewriter() override;
 
-  /// Apply a signature conversion to the entry block of the given region. This
-  /// replaces the entry block with a new block containing the updated
-  /// signature. The new entry block to the region is returned for convenience.
+  /// Apply a signature conversion to given block. This replaces the block with
+  /// a new block containing the updated signature. The operations of the given
+  /// block are inlined into the newly-created block, which is returned.
+  ///
   /// If no block argument types are changing, the entry original block will be
   /// left in place and returned.
   ///
-  /// If provided, `converter` will be used for any materializations.
+  /// A signature converison must be provided. (Type converters can construct
+  /// signature conversion with `convertBlockSignature`.) Optionally, a type
+  /// converter can be provided to build materializations.
----------------
matthias-springer wrote:

This part is still a bit unclear to me. (And I'm probably going to a look at the target/argument materialization logic separately.)

What I can say for sure is that the type converter is optional. We account for that case in `legalizeUnresolvedMaterialization`. Type converters are per-pattern. And `applySignatureConversion` is also called from patterns. So the pattern developer should know best if a type converter is needed or not. What's unclear to me is if users should generally provide a type convert "just in case". Most existing pattern (I checked MLIR and IREE) does not do that.

I expanded the documentation a bit to clarify that unresolved materializations may fail to legalize in the absence of a type converter.


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