Functions that materialize IR or convert types can be const.
Caching data structures inside the TypeConverter are marked as mutable.
Paths
| Differential D157597
[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 TimelineThis revision is now accepted and ready to land.Aug 10 2023, 3:29 AM Comment Actions
Actually for these particular cases, there is no tradeoff, it's a strict benefit to have const indeed. springerm added a child revision: D157601: [mlir][Conversion] Store const type converter in ConversionPattern.Aug 10 2023, 4:45 AM This revision was landed with ongoing or failed builds.Aug 10 2023, 4:59 AM Closed by commit rG3dd58333d09f: [mlir][Transforms] TypeConverter: Mark conversion/materialization functions as… (authored by springerm). · Explain Why This revision was automatically updated to reflect the committed changes. springerm removed a child revision: D157601: [mlir][Conversion] Store const type converter in ConversionPattern.Aug 10 2023, 6:17 AM Comment Actions 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 . This revision is now accepted and ready to land.Aug 16 2023, 11:34 AM This revision now requires changes to proceed.Aug 16 2023, 11:34 AM Comment Actions
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. Comment Actions 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 Comment Actions I don't understand how the concerns with respect to const in the IR constructs applies here, can someone elaborate? 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). Comment Actions 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.
Revision Contents
Diff 548981 mlir/include/mlir/Transforms/DialectConversion.h
mlir/lib/Transforms/Utils/DialectConversion.cpp
|