[mlir] Make LLVMIR Dialect types const correct

[zero9178]

Summary

As far as I am aware, not using const in MLIR only applies to Operations, not to types or attributes. Parts of the codebase currently assume const correctness of types. An example are type interfaces which only generate const methods.
This patch applies const to all immutable methods of the types in the LLVMIR Dialect, allowing type interfaces to be added in the future.

Notably, this was already done for LLVMPointerType back when the DataLayoutTypeInterface was added to it.

[nicolasvasilache]

Unless something changed recently, Type is a lightweight value type wrapping a pointer that is the uniqu'ed allocation of the type representation in the context.
Const does not mean much in this context and seems to go against the early rationale: https://mlir.llvm.org/docs/Rationale/UsageOfConst/.

Is a change of direction warranted here?
If so, the specific change and its scope should be well motivated and explained (in the doc I linked).

[nicolasvasilache]

Maybe one thing worth noting is that the doc I pointed to specifically talks about Operation and Value because these often require mutation and are linked in various ways with iplist.

Uniqued type and attributes are much simpler: they are immutable by construction and making a copy ends up just copying the underlying pointer.
So the fact that the doc seems to not discuss Type and Attribute is not that they should be treated differently, it's just that they are really trivial from a const perspective and there is no debate on constness in the first place.

[zero9178]

In D115596#3187779, @nicolasvasilache wrote:

Is a change of direction warranted here?
If so, the specific change and its scope should be well motivated and explained (in the doc I linked).

As far as I can tell this is not a change of direction as the doc only talks about Value and Operations. In fact, most Types getter methods I could find in MLIR are already const and tablegen marks such methods as const as well. The LLVMIR Dialect is the odd one out. So to me the direction seems to mark getter methods as const.

My motivation for the patch was https://reviews.llvm.org/D115600 in which I added a type interface implementation to LLVMStructType. Since interface methods are all generated as const by tablegen I'd have to write LLVMStructType notConst = *this; at the entry of each method. This seems unnecessary to me.

[mehdi_amini]

I'm fine with this patch, it seems trivial enough. But please wait for River to chime in as well.

[nicolasvasilache]

Since interface methods are all generated as const by tablegen I'd have to write LLVMStructType notConst = *this; at the entry of each method. This seems unnecessary to me.

Agreed about "seems unnecessary", still the tablegen blanket behavior seems fishy to me.
@rriddle knows best but since this seems to go against the spirit of const in MLIR, I'd be interested in seeing where this has been discussed when introduced.

[ftynse]

I added the first batch of const onto LLVM dialect types in https://github.com/llvm/llvm-project/commit/7325aaefa52a4bee91391cda2521006c31ab8010. The reason for this is how interfaces are implemented: they cannot call a non-const method on attributes/types. The whole const method situation with interfaces can be improved, in particular being able to specify const and non-const methods explicitly in ODS, and I think there is nothing fundamental preventing that.

My concerns with this patch are:

1. The rationale directly contradicts the "usage-of-const" document. It is not about making these types "more const-correct", which implies other types are not and should follow, it is about enabling them to be used with type interfaces.
2. As a corollary of the above, the patch can be amended to only change the methods that are being called from interfaces rather than do a blanket change for all of them.

[zero9178]

In D115596#3188549, @ftynse wrote:

1. The rationale directly contradicts the "usage-of-const" document. It is not about making these types "more const-correct", which implies other types are not and should follow, it is about enabling them to be used with type interfaces.

I am not 100% sure this is the case. The rationale mentions that it applies to core IR types. It later clarifies that it applies to "things like const Value and const Operation*", afterwards explicitly mentioning that its not meant for other types such as "the immutable types like Attribute, etc.".
As Type and Attribute are very similar it seems it might fall into this category.

I also just took a quick look over all type definitions in the mlir repo and found a mix of const and no const on various methods of Types for those defined in C++. Eg. FloatType does not have any const at all, others in BuiltinTypes.h only do on getters, SPIRV does on most but not all types. Types that are autogenerated by tablegen do have getters marked as const by default. So it seems to be quite inconsistent.

That said, I don't feel strongly about either direction and would be fine with simply marking the methods const that I need in https://reviews.llvm.org/D115600 for now. In the long term it'd probably be best to figure out whether types like Attribute and Type should be const correct, and if yes add the ability to interfaces to specify either const or non const. If Attribute and Type, like Ops, should not care about const at all it'd probably be easiest then to simply not make the generated interfaces const at all.

[ftynse]

In D115596#3188774, @zero9178 wrote:

In D115596#3188549, @ftynse wrote:

1. The rationale directly contradicts the "usage-of-const" document. It is not about making these types "more const-correct", which implies other types are not and should follow, it is about enabling them to be used with type interfaces.

I am not 100% sure this is the case. The rationale mentions that it applies to core IR types. It later clarifies that it applies to "things like const Value and const Operation*", afterwards explicitly mentioning that its not meant for other types such as "the immutable types like Attribute, etc.".
As Type and Attribute are very similar it seems it might fall into this category.

I also just took a quick look over all type definitions in the mlir repo and found a mix of const and no const on various methods of Types for those defined in C++. Eg. FloatType does not have any const at all, others in BuiltinTypes.h only do on getters, SPIRV does on most but not all types. Types that are autogenerated by tablegen do have getters marked as const by default. So it seems to be quite inconsistent.

This is precisely the issue: the usage of const on types is inconsistent. I am strongly supportive of making it consistent, but there is no consensus as to what the final state is. This patch pretends that the goal is to have as much const as possible on types. While this may well be the direction we will pursue, it deserves a much more visible discussion than a review thread on a non-user-facing dialect. I propose a pragmatic way forward: this patch is only needed to add one specific interface, that is a sufficient motivation for it to land. The discussion on how much const we put on types needs to happen on Discourse and make sure all dialect "owners" and code reviewers aware of that. I will be very happy if you lead that discussion and don't think this patch needs to be blocked on its outcome, but it will be if it is outgrowing its other motivation.

That said, I don't feel strongly about either direction and would be fine with simply marking the methods const that I need in https://reviews.llvm.org/D115600 for now. In the long term it'd probably be best to figure out whether types like Attribute and Type should be const correct, and if yes add the ability to interfaces to specify either const or non const. If Attribute and Type, like Ops, should not care about const at all it'd probably be easiest then to simply not make the generated interfaces const at all.

One thing that didn't age well about the const rationale: Attributes and Types are now mutable, and more safely than Values (which afford an unchecked type change).

[zero9178]

You are right. The situation here is less clear than I initially thought. I'll abandon this change and land https://reviews.llvm.org/D115600 with the minimum amount of const needed. Thanks a lot for the discussion!

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h

public:
 
   /// Gets or creates an instance of LLVM dialect array type containing
   /// `numElements` of `elementType`, in the same context as `elementType`.
   static LLVMArrayType get(Type elementType, unsigned numElements);
   static LLVMArrayType getChecked(function_ref<InFlightDiagnostic()> emitError,
                                   Type elementType, unsigned numElements);
 
   /// Returns the element type of the array.
-  Type getElementType();
+  Type getElementType() const;
 
   /// Returns the number of elements in the array type.
-  unsigned getNumElements();
+  unsigned getNumElements() const;
 
   /// Verifies that the type about to be constructed is well-formed.
   static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
                               Type elementType, unsigned numElements);
 };
 
 //===----------------------------------------------------------------------===//
 // LLVMFunctionType.
public:
 
   /// Checks if the given type can be used an argument in a function type.
   static bool isValidArgumentType(Type type);
 
   /// Checks if the given type can be used as a result in a function type.
   static bool isValidResultType(Type type);
 
   /// Returns whether the function is variadic.
-  bool isVarArg();
+  bool isVarArg() const;
 
   /// Gets or creates an instance of LLVM dialect function in the same context
   /// as the `result` type.
   static LLVMFunctionType get(Type result, ArrayRef<Type> arguments,
                               bool isVarArg = false);
   static LLVMFunctionType
   getChecked(function_ref<InFlightDiagnostic()> emitError, Type result,
              ArrayRef<Type> arguments, bool isVarArg = false);
 
   /// Returns the result type of the function.
-  Type getReturnType();
+  Type getReturnType() const;
 
   /// Returns the number of arguments to the function.
-  unsigned getNumParams();
+  unsigned getNumParams() const;
 
   /// Returns `i`-th argument of the function. Asserts on out-of-bounds.
-  Type getParamType(unsigned i);
+  Type getParamType(unsigned i) const;
 
   /// Returns a list of argument types of the function.
-  ArrayRef<Type> getParams();
-  ArrayRef<Type> params() { return getParams(); }
+  ArrayRef<Type> getParams() const;
+  ArrayRef<Type> params() const { return getParams(); }
 
   /// Verifies that the type about to be constructed is well-formed.
   static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
                               Type result, ArrayRef<Type> arguments, bool);
 };
 
 //===----------------------------------------------------------------------===//
 // LLVMPointerType.
public:
   /// not be set, e.g. if the struct already has a body or if it was marked as
   /// intentionally opaque. This might happen in a multi-threaded context when a
   /// different thread modified the struct after it was created. Most callers
   /// are likely to assert this always succeeds, but it is possible to implement
   /// a local renaming scheme based on the result of this call.
   LogicalResult setBody(ArrayRef<Type> types, bool isPacked);
 
   /// Checks if a struct is packed.
-  bool isPacked();
+  bool isPacked() const;
 
   /// Checks if a struct is identified.
-  bool isIdentified();
+  bool isIdentified() const;
 
   /// Checks if a struct is opaque.
-  bool isOpaque();
+  bool isOpaque() const;
 
   /// Checks if a struct is initialized.
-  bool isInitialized();
+  bool isInitialized() const;
 
   /// Returns the name of an identified struct.
-  StringRef getName();
+  StringRef getName() const;
 
   /// Returns the list of element types contained in a non-opaque struct.
-  ArrayRef<Type> getBody();
+  ArrayRef<Type> getBody() const;
 
   /// Verifies that the type about to be constructed is well-formed.
   static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
                               StringRef, bool);
   static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
                               ArrayRef<Type> types, bool);
 };
 
public:
 
   /// Support type casting functionality.
   static bool classof(Type type);
 
   /// Checks if the given type can be used in a vector type.
   static bool isValidElementType(Type type);
 
   /// Returns the element type of the vector.
-  Type getElementType();
+  Type getElementType() const;
 
   /// Returns the number of elements in the vector.
-  llvm::ElementCount getElementCount();
+  llvm::ElementCount getElementCount() const;
 
   /// Verifies that the type about to be constructed is well-formed.
   static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
                               Type elementType, unsigned numElements);
 };
 
 //===----------------------------------------------------------------------===//
 // LLVMFixedVectorType.
getChecked(function_ref<InFlightDiagnostic()> emitError, Type elementType,
              unsigned numElements);
 
   /// Checks if the given type can be used in a vector type. This type supports
   /// only a subset of LLVM dialect types that don't have a built-in
   /// counter-part, e.g., pointers.
   static bool isValidElementType(Type type);
 
   /// Returns the element type of the vector.
-  Type getElementType();
+  Type getElementType() const;
 
   /// Returns the number of elements in the fixed vector.
-  unsigned getNumElements();
+  unsigned getNumElements() const;
 
   /// Verifies that the type about to be constructed is well-formed.
   static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
                               Type elementType, unsigned numElements);
 };
 
 //===----------------------------------------------------------------------===//
 // LLVMScalableVectorType.
public:
   static LLVMScalableVectorType
   getChecked(function_ref<InFlightDiagnostic()> emitError, Type elementType,
              unsigned minNumElements);
 
   /// Checks if the given type can be used in a vector type.
   static bool isValidElementType(Type type);
 
   /// Returns the element type of the vector.
-  Type getElementType();
+  Type getElementType() const;
 
   /// Returns the scaling factor of the number of elements in the vector. The
   /// vector contains at least the resulting number of elements, or any non-zero
   /// multiple of this number.
-  unsigned getMinNumElements();
+  unsigned getMinNumElements() const;
 
   /// Verifies that the type about to be constructed is well-formed.
   static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
                               Type elementType, unsigned minNumElements);
 };
 
 //===----------------------------------------------------------------------===//
 // Printing and parsing.

mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp

 LLVMArrayType
 LLVMArrayType::getChecked(function_ref<InFlightDiagnostic()> emitError,
                           Type elementType, unsigned numElements) {
   assert(elementType && "expected non-null subtype");
   return Base::getChecked(emitError, elementType.getContext(), elementType,
                           numElements);
 }
 
-Type LLVMArrayType::getElementType() { return getImpl()->elementType; }
+Type LLVMArrayType::getElementType() const { return getImpl()->elementType; }
 
-unsigned LLVMArrayType::getNumElements() { return getImpl()->numElements; }
+unsigned LLVMArrayType::getNumElements() const {
+  return getImpl()->numElements;
+}
 
 LogicalResult
 LLVMArrayType::verify(function_ref<InFlightDiagnostic()> emitError,
                       Type elementType, unsigned numElements) {
   if (!isValidElementType(elementType))
     return emitError() << "invalid array element type: " << elementType;
   return success();
 }

 LLVMFunctionType::getChecked(function_ref<InFlightDiagnostic()> emitError,
                              Type result, ArrayRef<Type> arguments,
                              bool isVarArg) {
   assert(result && "expected non-null result");
   return Base::getChecked(emitError, result.getContext(), result, arguments,
                           isVarArg);
 }
 
-Type LLVMFunctionType::getReturnType() { return getImpl()->getReturnType(); }
+Type LLVMFunctionType::getReturnType() const {
+  return getImpl()->getReturnType();
+}
 
-unsigned LLVMFunctionType::getNumParams() {
+unsigned LLVMFunctionType::getNumParams() const {
   return getImpl()->getArgumentTypes().size();
 }
 
-Type LLVMFunctionType::getParamType(unsigned i) {
+Type LLVMFunctionType::getParamType(unsigned i) const {
   return getImpl()->getArgumentTypes()[i];
 }
 
-bool LLVMFunctionType::isVarArg() { return getImpl()->isVariadic(); }
+bool LLVMFunctionType::isVarArg() const { return getImpl()->isVariadic(); }
 
-ArrayRef<Type> LLVMFunctionType::getParams() {
+ArrayRef<Type> LLVMFunctionType::getParams() const {
   return getImpl()->getArgumentTypes();
 }
 
 LogicalResult
 LLVMFunctionType::verify(function_ref<InFlightDiagnostic()> emitError,
                          Type result, ArrayRef<Type> arguments, bool) {
   if (!isValidResultType(result))
     return emitError() << "invalid function result type: " << result;

 
 LogicalResult LLVMStructType::setBody(ArrayRef<Type> types, bool isPacked) {
   assert(isIdentified() && "can only set bodies of identified structs");
   assert(llvm::all_of(types, LLVMStructType::isValidElementType) &&
          "expected valid body types");
   return Base::mutate(types, isPacked);
 }
 
-bool LLVMStructType::isPacked() { return getImpl()->isPacked(); }
-bool LLVMStructType::isIdentified() { return getImpl()->isIdentified(); }
-bool LLVMStructType::isOpaque() {
+bool LLVMStructType::isPacked() const { return getImpl()->isPacked(); }
+bool LLVMStructType::isIdentified() const { return getImpl()->isIdentified(); }
+bool LLVMStructType::isOpaque() const {
   return getImpl()->isIdentified() &&
          (getImpl()->isOpaque() || !getImpl()->isInitialized());
 }
-bool LLVMStructType::isInitialized() { return getImpl()->isInitialized(); }
-StringRef LLVMStructType::getName() { return getImpl()->getIdentifier(); }
-ArrayRef<Type> LLVMStructType::getBody() {
+bool LLVMStructType::isInitialized() const {
+  return getImpl()->isInitialized();
+}
+StringRef LLVMStructType::getName() const { return getImpl()->getIdentifier(); }
+ArrayRef<Type> LLVMStructType::getBody() const {
   return isIdentified() ? getImpl()->getIdentifiedStructBody()
                         : getImpl()->getTypeList();
 }
 
 LogicalResult LLVMStructType::verify(function_ref<InFlightDiagnostic()>,
                                      StringRef, bool) {
   return success();
 }

 LLVMFixedVectorType
 LLVMFixedVectorType::getChecked(function_ref<InFlightDiagnostic()> emitError,
                                 Type elementType, unsigned numElements) {
   assert(elementType && "expected non-null subtype");
   return Base::getChecked(emitError, elementType.getContext(), elementType,
                           numElements);
 }
 
-Type LLVMFixedVectorType::getElementType() {
+Type LLVMFixedVectorType::getElementType() const {
   return static_cast<detail::LLVMTypeAndSizeStorage *>(impl)->elementType;
 }
 
-unsigned LLVMFixedVectorType::getNumElements() {
+unsigned LLVMFixedVectorType::getNumElements() const {
   return getImpl()->numElements;
 }
 
 bool LLVMFixedVectorType::isValidElementType(Type type) {
   return type.isa<LLVMPointerType, LLVMPPCFP128Type>();
 }
 
 LogicalResult

 LLVMScalableVectorType
 LLVMScalableVectorType::getChecked(function_ref<InFlightDiagnostic()> emitError,
                                    Type elementType, unsigned minNumElements) {
   assert(elementType && "expected non-null subtype");
   return Base::getChecked(emitError, elementType.getContext(), elementType,
                           minNumElements);
 }
 
-Type LLVMScalableVectorType::getElementType() {
+Type LLVMScalableVectorType::getElementType() const {
   return static_cast<detail::LLVMTypeAndSizeStorage *>(impl)->elementType;
 }
 
-unsigned LLVMScalableVectorType::getMinNumElements() {
+unsigned LLVMScalableVectorType::getMinNumElements() const {
   return getImpl()->numElements;
 }
 
 bool LLVMScalableVectorType::isValidElementType(Type type) {
   if (auto intType = type.dyn_cast<IntegerType>())
     return intType.isSignless();
 
   return isCompatibleFloatingPointType(type) || type.isa<LLVMPointerType>();