Remove the `conversionCallStack` from the MLIR TypeConverter

[mehdi_amini]

Summary

This vector keeps tracks of recursive types through the recursive invocations
of convertType(). However this is something only useful for some specific
cases, in which the dedicated conversion callbacks can handle this stack
privately.

This allows removing a mutable member of the type converter.

[springerm]

Looks like a good cleanup to me. (Regardless of whether this works in multi-threading or not.) Keeping the base type converter class simple and let derived classes handle recursion edge cases.

mlir/include/mlir/Conversion/LLVMCommon/TypeConverter.h

172

I would mention here that this currently only works for single-threaded execution.

I was looking for ways to make this work in a multi-threaded environment. The only solution that I found (without changing the API of convertTypes) is something like this. What do you think?

DenseMap<std::thread::id, std::unique_ptr<SmallVector<Type>>> conversionCallStack;
llvm::sys::SmartRWMutex<true> callStackMutex;

...

  addConversion([&](LLVM::LLVMStructType type, SmallVectorImpl<Type> &results)
                    -> std::optional<LogicalResult> {
    ...
    SmallVector<Type> *stack = nullptr;
    std::shared_lock stackReadLock(callStackMutex);
    stackReadLock.lock();
    auto it = conversionCallStack.find(std::this_thread::get_id());
    if (it != conversionCallStack.end())
      stack = it.second().get();
    stackReadLock.unlock();
    if (!stack) {
      std::unique_lock stackWriteLock(callStackMutex);
      stackWriteLock.lock();
      stack = conversionCallStack.try_emplace(std::this_thread::get_id()).first.get();
    }
     
    // `stack` can be used without further synchronization/locking. 
    stack->push_back(type);
    auto popConversionCallStack =
        llvm::make_scope_exit([this]() { conversionCallStack->pop_back(); });

  }

173

The same field is needed in flang/lib/Optimizer/CodeGen/TypeConverter.cpp.

[springerm]

mlir/include/mlir/Conversion/LLVMCommon/TypeConverter.h

172

I think this would also work and does not require locking:

private:
  static thread_local DenseMap<LLVMTypeConverter *, SmallVector<Type>> conversionCallStack;

~LLVMTypeConverter() {
  conversionCallStack.erase(this);
}

addConversion([&](...) {
  SmallVector<Type> &stack = conversionCallStack[this];
  // `stack` can be used without locking.
});

[mehdi_amini]

mlir/include/mlir/Conversion/LLVMCommon/TypeConverter.h

172

This is still "global mutable state", which I find unsatisfactory as a general approach though.

[mehdi_amini]

Add per-thread handling and locking

[mehdi_amini]

mlir/include/mlir/Conversion/LLVMCommon/TypeConverter.h

173

They inherit from the class here, so it should be all fine!

[mehdi_amini]

Fix flang

[mehdi_amini]

mlir/include/mlir/Conversion/LLVMCommon/TypeConverter.h

173

Actually I see what you mean, I fixed flang :)