Page MenuHomePhabricator

[mlir] Make LLVMIR Dialect types const correct
AbandonedPublic

Authored by zero9178 on Dec 12 2021, 3:40 AM.

Details

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.

Diff Detail

Event Timeline

zero9178 created this revision.Dec 12 2021, 3:40 AM
zero9178 requested review of this revision.Dec 12 2021, 3:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2021, 3:40 AM

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).

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.

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 accepted this revision.Dec 12 2021, 11:16 AM

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

This revision is now accepted and ready to land.Dec 12 2021, 11:16 AM

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.

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.
  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.

  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 abandoned this revision.Dec 13 2021, 5:57 AM

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!