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.
@MaheshRavishankar said:
“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 (Integrate llvm-project at 030e315ee771 by hanhanW · Pull Request #14678 · openxla/iree · GitHub) . I dont think having const this way is worth the effort, due to a reasoning similar to what is provided here Usage of 'const' in MLIR, for core IR types - MLIR . We are basically hitting this case Usage of 'const' in MLIR, for core IR types - MLIR .”
@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)."