[mlir][Transforms][NFC] TypeConverter: Mark conversion/materialization functions as "const"

[springerm]

Summary

Functions that materialize IR or convert types can be const.

Caching data structures inside the TypeConverter are marked as mutable.

[nicolasvasilache]

I am personally strong +1 on this but let's wait to see what others say here.

[mehdi_amini]

More const seems better when possible. Do I miss a drawback of this patch?

[nicolasvasilache]

In D157597#4576248, @mehdi_amini wrote:

More const seems better when possible. Do I miss a drawback of this patch?

Actually for these particular cases, there is no tradeoff, it's a strict benefit to have const indeed.

[mravishankar]

I missed this patch, but I would recommend reverting this commit. I understand the value of const, but it also requires const to be plumbed all the way through properly. Downstream we had issues integrating this change (https://github.com/openxla/iree/pull/14678#discussion_r1295052608) . I dont think having const this way is worth the effort, due to a reasoning similar to what is provided here https://mlir.llvm.org/docs/Rationale/UsageOfConst/ . We are basically hitting this case https://mlir.llvm.org/docs/Rationale/UsageOfConst/#const-incorrect-in-practice .

[stellaraccident]

In D157597#4592841, @mravishankar wrote:

I missed this patch, but I would recommend reverting this commit. I understand the value of const, but it also requires const to be plumbed all the way through properly. Downstream we had issues integrating this change (https://github.com/openxla/iree/pull/14678#discussion_r1295052608) . I dont think having const this way is worth the effort, due to a reasoning similar to what is provided here https://mlir.llvm.org/docs/Rationale/UsageOfConst/ . We are basically hitting this case https://mlir.llvm.org/docs/Rationale/UsageOfConst/#const-incorrect-in-practice .

I had a similar reaction on the first read. Personally, between the Rationale/UsageOfConst and the use of mutable, with no other context, I would call this out of bounds and not a good use of const for C++. Might just be personal preference, but I've always taken a dim view of const interfaces that require use of mutable as a code smell that the API is wrong. In this case conversionCallStack is an indication that this is not even just the caching case that mutable is generally accepted for: this is a stateful class by design and making it const hides that.

I vote for revert.

[springerm]

This change was partly motivated due to a few hacks around the usage of the type converter that were possible because it was not marked as const. There's one example in the GPU dialect: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Conversion/LLVMCommon/TypeConverter.h#L98 (this is called in GPUToLLVMConversion.cpp, I cleaned it up a little bit in D157601). Another questionable piece of code is the TBAABuilder in Flang (which is part of the type converter there), which updates its internal state with every type conversion.

Regarding usage of const in general, also see the discussion on D157601.

There is also some more background about this change here: https://discourse.llvm.org/t/rfc-almost-all-uses-of-typeconverter-should-be-const/72689

[mehdi_amini]

I don't understand how the concerns with respect to const in the IR constructs applies here, can someone elaborate?
We took a stance on IR construct, but I haven't seen this extended to anything else so far and I'd be concern if this started to get viral beyond the IR.

Mutability for caching could be acceptable, except that it makes it hard to reason about thread safety (a const T & can't really be used across thread safely, naively at least).

[mravishankar]

Moving the discussion to https://discourse.llvm.org/t/rfc-almost-all-uses-of-typeconverter-should-be-const/72689/3 . I copied all the comments from here to the RFC.

mlir/include/mlir/Transforms/DialectConversion.h

void addTypeAttributeConversion(FnT &&callback) {
     registerTypeAttributeConversion(
         wrapTypeAttributeConversion<T, A>(std::forward<FnT>(callback)));
   }
 
   /// Convert the given type. This function should return failure if no valid
   /// conversion exists, success otherwise. If the new set of types is empty,
   /// the type is removed and any usages of the existing value are expected to
   /// be removed during conversion.
-  LogicalResult convertType(Type t, SmallVectorImpl<Type> &results);
+  LogicalResult convertType(Type t, SmallVectorImpl<Type> &results) const;
 
   /// This hook simplifies defining 1-1 type conversions. This function returns
   /// the type to convert to on success, and a null type on failure.
-  Type convertType(Type t);
+  Type convertType(Type t) const;
 
   /// Attempts a 1-1 type conversion, expecting the result type to be
   /// `TargetType`. Returns the converted type cast to `TargetType` on success,
   /// and a null type on conversion or cast failure.
-  template <typename TargetType>
-  TargetType convertType(Type t) {
+  template <typename TargetType> TargetType convertType(Type t) const {
     return dyn_cast_or_null<TargetType>(convertType(t));
   }
 
   /// Convert the given set of types, filling 'results' as necessary. This
   /// returns failure if the conversion of any of the types fails, success
   /// otherwise.
-  LogicalResult convertTypes(TypeRange types, SmallVectorImpl<Type> &results);
+  LogicalResult convertTypes(TypeRange types,
+                             SmallVectorImpl<Type> &results) const;
 
   /// Return true if the given type is legal for this type converter, i.e. the
   /// type converts to itself.
-  bool isLegal(Type type);
+  bool isLegal(Type type) const;
+
   /// Return true if all of the given types are legal for this type converter.
   template <typename RangeT>
   std::enable_if_t<!std::is_convertible<RangeT, Type>::value &&
                        !std::is_convertible<RangeT, Operation *>::value,
                    bool>
-  isLegal(RangeT &&range) {
+  isLegal(RangeT &&range) const {
     return llvm::all_of(range, [this](Type type) { return isLegal(type); });
   }
   /// Return true if the given operation has legal operand and result types.
-  bool isLegal(Operation *op);
+  bool isLegal(Operation *op) const;
 
   /// Return true if the types of block arguments within the region are legal.
-  bool isLegal(Region *region);
+  bool isLegal(Region *region) const;
 
   /// Return true if the inputs and outputs of the given function type are
   /// legal.
-  bool isSignatureLegal(FunctionType ty);
+  bool isSignatureLegal(FunctionType ty) const;
 
   /// This method allows for converting a specific argument of a signature. It
   /// takes as inputs the original argument input number, type.
   /// On success, it populates 'result' with any new mappings.
   LogicalResult convertSignatureArg(unsigned inputNo, Type type,
-                                    SignatureConversion &result);
+                                    SignatureConversion &result) const;
   LogicalResult convertSignatureArgs(TypeRange types,
                                      SignatureConversion &result,
-                                     unsigned origInputOffset = 0);
+                                     unsigned origInputOffset = 0) const;
 
   /// This function converts the type signature of the given block, by invoking
   /// 'convertSignatureArg' for each argument. This function should return a
   /// valid conversion for the signature on success, std::nullopt otherwise.
-  std::optional<SignatureConversion> convertBlockSignature(Block *block);
+  std::optional<SignatureConversion> convertBlockSignature(Block *block) const;
 
   /// Materialize a conversion from a set of types into one result type by
   /// generating a cast sequence of some kind. See the respective
   /// `add*Materialization` for more information on the context for these
   /// methods.
   Value materializeArgumentConversion(OpBuilder &builder, Location loc,
-                                      Type resultType, ValueRange inputs) {
+                                      Type resultType,
+                                      ValueRange inputs) const {
     return materializeConversion(argumentMaterializations, builder, loc,
                                  resultType, inputs);
   }
   Value materializeSourceConversion(OpBuilder &builder, Location loc,
-                                    Type resultType, ValueRange inputs) {
+                                    Type resultType, ValueRange inputs) const {
     return materializeConversion(sourceMaterializations, builder, loc,
                                  resultType, inputs);
   }
   Value materializeTargetConversion(OpBuilder &builder, Location loc,
-                                    Type resultType, ValueRange inputs) {
+                                    Type resultType, ValueRange inputs) const {
     return materializeConversion(targetMaterializations, builder, loc,
                                  resultType, inputs);
   }
 
   /// Convert an attribute present `attr` from within the type `type` using
   /// the registered conversion functions. If no applicable conversion has been
   /// registered, return std::nullopt. Note that the empty attribute/`nullptr`
   /// is a valid return value for this function.
-  std::optional<Attribute> convertTypeAttribute(Type type, Attribute attr);
+  std::optional<Attribute> convertTypeAttribute(Type type,
+                                                Attribute attr) const;
 
 private:
   /// The signature of the callback used to convert a type. If the new set of
   /// types is empty, the type is removed and any usages of the existing value
   /// are expected to be removed during conversion.
   using ConversionCallbackFn = std::function<std::optional<LogicalResult>(
       Type, SmallVectorImpl<Type> &, ArrayRef<Type>)>;
 
   /// The signature of the callback used to materialize a conversion.
   using MaterializationCallbackFn = std::function<std::optional<Value>(
       OpBuilder &, Type, ValueRange, Location)>;
 
   /// The signature of the callback used to convert a type attribute.
   using TypeAttributeConversionCallbackFn =
       std::function<AttributeConversionResult(Type, Attribute)>;
 
   /// Attempt to materialize a conversion using one of the provided
   /// materialization functions.
-  Value materializeConversion(
-      MutableArrayRef<MaterializationCallbackFn> materializations,
-      OpBuilder &builder, Location loc, Type resultType, ValueRange inputs);
+  Value
+  materializeConversion(ArrayRef<MaterializationCallbackFn> materializations,
+                        OpBuilder &builder, Location loc, Type resultType,
+                        ValueRange inputs) const;
 
   /// Generate a wrapper for the given callback. This allows for accepting
   /// different callback forms, that all compose into a single version.
   /// With callback of form: `std::optional<Type>(T)`
   template <typename T, typename FnT>
   std::enable_if_t<std::is_invocable_v<FnT, T>, ConversionCallbackFn>
-  wrapCallback(FnT &&callback) {
+  wrapCallback(FnT &&callback) const {
     return wrapCallback<T>(
         [callback = std::forward<FnT>(callback)](
             T type, SmallVectorImpl<Type> &results, ArrayRef<Type>) {
           if (std::optional<Type> resultOpt = callback(type)) {
             bool wasSuccess = static_cast<bool>(*resultOpt);
             if (wasSuccess)
               results.push_back(*resultOpt);
             return std::optional<LogicalResult>(success(wasSuccess));
           }
           return std::optional<LogicalResult>();
         });
   }
   /// With callback of form: `std::optional<LogicalResult>(
   ///     T, SmallVectorImpl<Type> &)`.
   template <typename T, typename FnT>
   std::enable_if_t<std::is_invocable_v<FnT, T, SmallVectorImpl<Type> &>,
                    ConversionCallbackFn>
-  wrapCallback(FnT &&callback) {
+  wrapCallback(FnT &&callback) const {
     return wrapCallback<T>(
         [callback = std::forward<FnT>(callback)](
             T type, SmallVectorImpl<Type> &results, ArrayRef<Type>) {
           return callback(type, results);
         });
   }
   /// With callback of form: `std::optional<LogicalResult>(
   ///     T, SmallVectorImpl<Type> &, ArrayRef<Type>)`.
   template <typename T, typename FnT>
   std::enable_if_t<
       std::is_invocable_v<FnT, T, SmallVectorImpl<Type> &, ArrayRef<Type>>,
       ConversionCallbackFn>
-  wrapCallback(FnT &&callback) {
+  wrapCallback(FnT &&callback) const {
     return [callback = std::forward<FnT>(callback)](
                Type type, SmallVectorImpl<Type> &results,
                ArrayRef<Type> callStack) -> std::optional<LogicalResult> {
       T derivedType = dyn_cast<T>(type);
       if (!derivedType)
         return std::nullopt;
       return callback(derivedType, results, callStack);
     };
   }
 
   /// Register a type conversion.
   void registerConversion(ConversionCallbackFn callback) {
     conversions.emplace_back(std::move(callback));
     cachedDirectConversions.clear();
     cachedMultiConversions.clear();
   }
 
   /// Generate a wrapper for the given materialization callback. The callback
   /// may take any subclass of `Type` and the wrapper will check for the target
   /// type to be of the expected class before calling the callback.
   template <typename T, typename FnT>
-  MaterializationCallbackFn wrapMaterialization(FnT &&callback) {
+  MaterializationCallbackFn wrapMaterialization(FnT &&callback) const {
     return [callback = std::forward<FnT>(callback)](
                OpBuilder &builder, Type resultType, ValueRange inputs,
                Location loc) -> std::optional<Value> {
       if (T derivedType = dyn_cast<T>(resultType))
         return callback(builder, derivedType, inputs, loc);
       return std::nullopt;
     };
   }
 
   /// Generate a wrapper for the given memory space conversion callback. The
   /// callback may take any subclass of `Attribute` and the wrapper will check
   /// for the target attribute to be of the expected class before calling the
   /// callback.
   template <typename T, typename A, typename FnT>
   TypeAttributeConversionCallbackFn
-  wrapTypeAttributeConversion(FnT &&callback) {
+  wrapTypeAttributeConversion(FnT &&callback) const {
     return [callback = std::forward<FnT>(callback)](
                Type type, Attribute attr) -> AttributeConversionResult {
       if (T derivedType = dyn_cast<T>(type)) {
         if (A derivedAttr = dyn_cast_or_null<A>(attr))
           return callback(derivedType, derivedAttr);
       }
       return AttributeConversionResult::na();
     };
private:
   SmallVector<MaterializationCallbackFn, 2> targetMaterializations;
 
   /// The list of registered type attribute conversion functions.
   SmallVector<TypeAttributeConversionCallbackFn, 2> typeAttributeConversions;
 
   /// A set of cached conversions to avoid recomputing in the common case.
   /// Direct 1-1 conversions are the most common, so this cache stores the
   /// successful 1-1 conversions as well as all failed conversions.
-  DenseMap<Type, Type> cachedDirectConversions;
+  mutable DenseMap<Type, Type> cachedDirectConversions;
   /// This cache stores the successful 1->N conversions, where N != 1.
-  DenseMap<Type, SmallVector<Type, 2>> cachedMultiConversions;
+  mutable DenseMap<Type, SmallVector<Type, 2>> cachedMultiConversions;
 
   /// Stores the types that are being converted in the case when convertType
   /// is being called recursively to convert nested types.
-  SmallVector<Type, 2> conversionCallStack;
+  mutable SmallVector<Type, 2> conversionCallStack;
 };
 
 //===----------------------------------------------------------------------===//
 // Conversion Patterns
 //===----------------------------------------------------------------------===//
 
 /// Base class for the conversion patterns. This pattern class enables type
 /// conversions, and other uses specific to the conversion framework. As such,

mlir/lib/Transforms/Utils/DialectConversion.cpp

 void TypeConverter::SignatureConversion::remapInput(unsigned origInputNo,
                                                     Value replacementValue) {
   assert(!remappedInputs[origInputNo] && "input has already been remapped");
   remappedInputs[origInputNo] =
       InputMapping{origInputNo, /*size=*/0, replacementValue};
 }
 
 LogicalResult TypeConverter::convertType(Type t,
-                                         SmallVectorImpl<Type> &results) {
+                                         SmallVectorImpl<Type> &results) const {
   auto existingIt = cachedDirectConversions.find(t);
   if (existingIt != cachedDirectConversions.end()) {
     if (existingIt->second)
       results.push_back(existingIt->second);
     return success(existingIt->second != nullptr);
   }
   auto multiIt = cachedMultiConversions.find(t);
   if (multiIt != cachedMultiConversions.end()) {
     results.append(multiIt->second.begin(), multiIt->second.end());
     return success();
   }
 
   // Walk the added converters in reverse order to apply the most recently
   // registered first.
   size_t currentCount = results.size();
   conversionCallStack.push_back(t);
   auto popConversionCallStack =
       llvm::make_scope_exit([this]() { conversionCallStack.pop_back(); });
-  for (ConversionCallbackFn &converter : llvm::reverse(conversions)) {
+  for (const ConversionCallbackFn &converter : llvm::reverse(conversions)) {
     if (std::optional<LogicalResult> result =
             converter(t, results, conversionCallStack)) {
       if (!succeeded(*result)) {
         cachedDirectConversions.try_emplace(t, nullptr);
         return failure();
       }
       auto newTypes = ArrayRef<Type>(results).drop_front(currentCount);
       if (newTypes.size() == 1)
         cachedDirectConversions.try_emplace(t, newTypes.front());
       else
         cachedMultiConversions.try_emplace(t, llvm::to_vector<2>(newTypes));
       return success();
     }
   }
   return failure();
 }
 
-Type TypeConverter::convertType(Type t) {
+Type TypeConverter::convertType(Type t) const {
   // Use the multi-type result version to convert the type.
   SmallVector<Type, 1> results;
   if (failed(convertType(t, results)))
     return nullptr;
 
   // Check to ensure that only one type was produced.
   return results.size() == 1 ? results.front() : nullptr;
 }
 
-LogicalResult TypeConverter::convertTypes(TypeRange types,
-                                          SmallVectorImpl<Type> &results) {
+LogicalResult
+TypeConverter::convertTypes(TypeRange types,
+                            SmallVectorImpl<Type> &results) const {
   for (Type type : types)
     if (failed(convertType(type, results)))
       return failure();
   return success();
 }
 
-bool TypeConverter::isLegal(Type type) { return convertType(type) == type; }
-bool TypeConverter::isLegal(Operation *op) {
+bool TypeConverter::isLegal(Type type) const {
+  return convertType(type) == type;
+}
+bool TypeConverter::isLegal(Operation *op) const {
   return isLegal(op->getOperandTypes()) && isLegal(op->getResultTypes());
 }
 
-bool TypeConverter::isLegal(Region *region) {
+bool TypeConverter::isLegal(Region *region) const {
   return llvm::all_of(*region, [this](Block &block) {
     return isLegal(block.getArgumentTypes());
   });
 }
 
-bool TypeConverter::isSignatureLegal(FunctionType ty) {
+bool TypeConverter::isSignatureLegal(FunctionType ty) const {
   return isLegal(llvm::concat<const Type>(ty.getInputs(), ty.getResults()));
 }
 
-LogicalResult TypeConverter::convertSignatureArg(unsigned inputNo, Type type,
-                                                 SignatureConversion &result) {
+LogicalResult
+TypeConverter::convertSignatureArg(unsigned inputNo, Type type,
+                                   SignatureConversion &result) const {
   // Try to convert the given input type.
   SmallVector<Type, 1> convertedTypes;
   if (failed(convertType(type, convertedTypes)))
     return failure();
 
   // If this argument is being dropped, there is nothing left to do.
   if (convertedTypes.empty())
     return success();
 
   // Otherwise, add the new inputs.
   result.addInputs(inputNo, convertedTypes);
   return success();
 }
-LogicalResult TypeConverter::convertSignatureArgs(TypeRange types,
-                                                  SignatureConversion &result,
-                                                  unsigned origInputOffset) {
+LogicalResult
+TypeConverter::convertSignatureArgs(TypeRange types,
+                                    unsigned origInputOffset) const {
   for (unsigned i = 0, e = types.size(); i != e; ++i)
     if (failed(convertSignatureArg(origInputOffset + i, types[i], result)))
       return failure();
   return success();
 }
 
 Value TypeConverter::materializeConversion(
-    MutableArrayRef<MaterializationCallbackFn> materializations,
-    OpBuilder &builder, Location loc, Type resultType, ValueRange inputs) {
-  for (MaterializationCallbackFn &fn : llvm::reverse(materializations))
+    ArrayRef<MaterializationCallbackFn> materializations, OpBuilder &builder,
+    Location loc, Type resultType, ValueRange inputs) const {
+  for (const MaterializationCallbackFn &fn : llvm::reverse(materializations))
     if (std::optional<Value> result = fn(builder, resultType, inputs, loc))
       return *result;
   return nullptr;
 }
 
-auto TypeConverter::convertBlockSignature(Block *block)
-    -> std::optional<SignatureConversion> {
+std::optional<TypeConverter::SignatureConversion>
+TypeConverter::convertBlockSignature(Block *block) const {
   SignatureConversion conversion(block->getNumArguments());
   if (failed(convertSignatureArgs(block->getArgumentTypes(), conversion)))
     return std::nullopt;
   return conversion;
 }
 
 //===----------------------------------------------------------------------===//
 // Type attribute conversion
bool TypeConverter::AttributeConversionResult::isAbort() const {
   return impl.getInt() == abortTag;
 }
 
 Attribute TypeConverter::AttributeConversionResult::getResult() const {
   assert(hasResult() && "Cannot get result from N/A or abort");
   return impl.getPointer();
 }
 
-std::optional<Attribute> TypeConverter::convertTypeAttribute(Type type,
-                                                             Attribute attr) {
-  for (TypeAttributeConversionCallbackFn &fn :
+std::optional<Attribute>
+TypeConverter::convertTypeAttribute(Type type, Attribute attr) const {
+  for (const TypeAttributeConversionCallbackFn &fn :
        llvm::reverse(typeAttributeConversions)) {
     AttributeConversionResult res = fn(type, attr);
     if (res.hasResult())
       return res.getResult();
     if (res.isAbort())
       return std::nullopt;
   }
   return std::nullopt;