[RFC] Almost all uses of TypeConverter should be const

[matthias-springer]

@MaheshRavishankar

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.

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.

@stellaraccident

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.

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!