This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by springerm on Aug 10 2023, 3:15 AM.

Details

Summary

Functions that materialize IR or convert types can be const.

Caching data structures inside the TypeConverter are marked as mutable.

Diff Detail

Event Timeline

springerm created this revision.Aug 10 2023, 3:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2023, 3:15 AM
springerm requested review of this revision.Aug 10 2023, 3:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2023, 3:15 AM
springerm edited the summary of this revision. (Show Details)Aug 10 2023, 3:15 AM
springerm added a reviewer: mehdi_amini.
nicolasvasilache accepted this revision.Aug 10 2023, 3:29 AM

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

This revision is now accepted and ready to land.Aug 10 2023, 3:29 AM
mehdi_amini accepted this revision.Aug 10 2023, 3:51 AM

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

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.

This revision was landed with ongoing or failed builds.Aug 10 2023, 4:59 AM
This revision was automatically updated to reflect the committed changes.

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 .

mravishankar reopened this revision.Aug 16 2023, 11:34 AM
This revision is now accepted and ready to land.Aug 16 2023, 11:34 AM
mravishankar requested changes to this revision.Aug 16 2023, 11:34 AM
This revision now requires changes to proceed.Aug 16 2023, 11:34 AM

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.

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

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

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.