I believe there are a few holes around uses of TypeConverter that can go quite wrong.
I have been exploring better ways to centralize the type conversion behavior and realized that we have not been using const correctness at all for those potentially surprising interface points.
I believe we should consider make the following changes:
populateXXXConversionPatterns should take const TypeConverter. ⚙ D157501 [mlir][transform] Add NVGPU to NVVM conversion via transform.apply_conversion_patterns shed some light on the fact that some populatePatterns implementations may decide to modify the type converter and we should explicitly disallow that. This would be particularly bad and surprising when combined with 2. (in particular, the current implementation in that PR is error-prone and needs to be fixed).
We should not allow non-determinism in TypeConverter depending on the order in which populate functions are applied. At the moment, the last one to add a conversion for a particular type “wins”. Instead, we should fail hard with a clear error message.
TypeConverter should be built ahead of time and then only used with const everywhere. Almost all methods should be updated to use const. Performance-related internal caching implementation details should be done with mutable.
A related aspect is that the conversion passes create their own TypeConverter from their independent (and sometime inconsistent) pass options. This can make it hard to make all these passes agree, which yields to surprising unrealized conversion casts. As such, existing conversion passes can at best be described as “test” passes that should not be combined into complex pipelines.
It would also be great to come up with a separate TypeConversion configuration mechanism (e.g. from data layout) that would be centralized and orthogonal to conversion passes but I do not have concrete proposals here to make this work unsurprisingly with all the pass machinery.
For now we are iterating on making conversions more user-friendly, thanks to configurable apply_conversion_pattern ops in the transform dialect (e.g. here). This work has led to uncovering a bunch of issues that prompted this post.
Lastly, I believe there are also a bunch of questions related to the “context-free” aspect of type derivation in the conversion patterns. But, I think we can make significant strides with better matching (i.e. a much finer-grain targeting of ops + conditions on which to apply very targeted declarative conversions).
This change makes the TypeConverter * that is stored in ConversionPattern constant: ⚙ D157601 [mlir][Conversion] Store const type converter in ConversionPattern. There is no code in MLIR that modifies a type converter once a dialect conversion has started; this change basically just updates function signatures.
There is one place in Flang where a type converter is modified while a dialect conversion is running, but this can be fixed: ⚙ D157611 [flang][NFC] Wrap TBAABuilder in unique_ptr. (They are not changing the actual type conversion rules. They just store some extra mutable state that could also be stored elsewhere.)
Have a bit distracted lately, so missed this RFC. It came up while trying to integrate this commit downstream. I am moving the discussion from the patch to this RFC. So following is a copy of the comments from the patch.
@stellaraccident said:
“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.”
@matthias-springer said:
" 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."
@mehdi_amini said:
"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)."
To me this change has the same issue as the argument for IR constructs. const requires things to be fully plumbed through, and sometimes legitimate uses of methods get blocked because the method is not marked const, requiring you to use const_cast. So to me it is about what is better. I definitely see some of the TypeConverter methods are being abused (and doing non-trivial things), but I dont see much value in trying to use const to make such implementations illegal. It causes too much collateral damage due to some of the same reasons as Usage of 'const' in MLIR, for core IR types - MLIR.
I don’t want to get into a debate on the proper use of c++ const correctness. My personal opinion is that it is unusable for all but the simplest things, but I won’t hold to that strongly, because then I’d be debating as mentioned UT, using mutable for caching isn’t great because it makes threading harder to reason about and leads to contradictory looking things (ie. Why do I have to take a lock to share this immutable looking instance). But again, that is debate territory.
I haven’t looked at it too closely but the presence of conversionCallStacklooks to be a straight up mutable API contract with a caller. And I think that is out of bounds of what most people would say mutable should be used for… This is not an immutable object…
I don’t care strongly one way or the other, but I don’t understand the rationale around citing “Usage of ‘const’ in MLIR, for core IR types”. The TypeConverter is /not/ part of the IR and doesn’t have the same considerations - we don’t apply those rules to the pass manager or other things that are “part of MLIR but not the IR itself”.
It is very idiomatic C++ to use ‘mutable’ for caches in logically const methods. This is basically the reason the feature exists.
EDIT: I just looked at the patch, and I do agree that marking ‘conversionCallStack’ as mutable is quite “sus” (as my kids would say :). The other uses of mutable seem fine though,
The way that the conversionCallStack is used here is triggering a code smell to me, but I can see on closer look that it is part of the caching algorithm and is (mostly) a local effect. That was my only real observation.
Chris: I suspect that some of us remember the… strong feelings… that went into that rationale and perhaps interpreted the intent a bit more broadly Especially with a background on more threaded designs, I generally prefer that const implies immutable and have never liked the caching carveout because it obfuscates locking behavior. But that is preference… and doesn’t apply here.
As a note, I would like to have a type conversion setup mechanism that’s less fragile when dealing with compiling to AMD GPU targets (aka ROCDL). Specifically, I need to override the LLVM type converter’s conversion of bf16 to itself: I need bf16 to translate to i16 for our backend to behave correctly.
Looking at the IREE integration change above, the const_cast can be avoided by making the caching data structure (typeOrdinalMap) mutable. This (having a const method that computes something and having a mutable cache) is a pattern that I have seen in other parts of the MLIR code base, most notably DominanceInfo and DataLayout.
conversionCallStack is used by the LLVM type converter to handle recursive types. It is indeed a bit different from the other caching data structures:
The conversion type caches are “insert+read only”. We never remove/change existing elements in the cache. This should make it quite easy to make this cache thread-safe.
The conversionCallStack only makes sense per thread. Each thread is pushing/popping “stack frames”. This was already a problem (in terms of thread safety) before I made the type converter const.
Ideally, I would replace with conversionCallStack with a new object (SmallVector<Type> or something similar) that can optionally be passed to TypeConverter::convertType and will be accessible as an extra argument in the type conversion lambda. Each type conversion lambda can inspect the call stack and decide what to do in case of a recursive call.
Btw, I ran into the same problem when implementing BufferizableOpInterface::getBufferType and decided to pass along an extra invocationStack variable with each call/interface implementation.
If somebody knows a better C++ solution, please let me know!
Const in C++ remains a mess. There are definitely minor problems with the current design even for IR, there are cases where you can get a ‘declared const’ IR value back through composition. I don’t recall exactly where, but I think I’ve run into cases where you run a zip iterator with an operand list and something else… and it gives you ‘const tuples’ back. The problem with this is that extracting a value out of a const tuple gives you a ‘const Value’ which you can’t use mutable accessor with, without weird workarounds like ((Value)expressionReturningConstValue).getType() or assigning to a local Value on the stack and then using it.
All that said, I think the rationale in the doc is still valid: it isn’t whether we have a beautiful and ideal solution - such a thing isn’t possible. The question is whether we have the “least bad” solution. I think our approach is the least bad because the super edge cases like the thing above happen very rarely, and the traditional C++ use of const would require massive code duplication that would affect build time and code size in the common case in practice.
I can understand how the discussion is nuanced and perhaps traumatic for the folks who went through it back in the day, but it is highly motivated by the specific problems described in the rationale, and they don’t apply to general infrastructure-that-isn’t-itself-IR like this a PassManager or this type.
Oh I’ve definitely run into things like that and then “un-const’d” entire hierarchies in a fit of not wanting to read another five pages of error messages. We’ve all got our personal line on where “enough trouble” is on this feature and mine is pretty low and hews more towards “this thing is immutable” vs a logical abstraction. But I don’t have a horse in this debate – and try not to debate such things if a precedent is established. Thanks for explaining the intended line.
In this specific case, that recursive type cache thing still has me scratching my head, and it seems like, even const-ness aside, could use a rethink. I don’t have a specific suggestion though.
Before you made this “const” the thread safety was less of a concerns, because you can’t in general use a mutable object without thinking about thread safety, right now we’re having a footgun.
That does not seem like a safe API to me, it’s too easy to “forget” to carry the state over unfortunately.
The conversionCallStack is not caching here: it is really necessary state for the recursive algorithm and it really is not an appropriate use of the mutable keyword (contrary to the two other “caching” structure).
Unfortunately I can’t seem to figure out how to extract this out, because it is either gonna impact a lot of APIs, make the API fragile, and/or because of the way the TypeConverter is actually inherited everywhere.
as mentioned UT, using mutable for caching isn’t great because it makes threading harder to reason about and leads to contradictory looking things (ie. Why do I have to take a lock to share this immutable looking instance). But again, that is debate territory.
Especially with a background on more threaded designs, I generally prefer that