[Mlir-commits] [mlir] [mlir] Use non thread-local allocator for DistinctAttr when threading is disabled (PR #128566)

[Artemiy Bulavin]

abulavin wrote:

> How much of a burden would it be to just enforce no multi-threading when doing crash reproducers?

This has pass pipeline performance implications for downstream projects that have crash reproduction always on. For example, the downstream project https://github.com/triton-lang/triton has crash reproducer always enabled when running the pass manager, as you can see here: https://github.com/triton-lang/triton/blob/main/python/src/ir.cc#L1817-L1839. If we enforce multithreading to be disabled then projects like this will not be able to benefit from pass parallelisation.

I appreciate that disabling/enabling thread local storage does add complexity however my intention with this PR was not to necessarily make this part of the `MLIRContext`'s public interface. It's only really the pass manager that needs to disable thread-local storage for safety reasons but I could not find a nice way to do this without affecting `MLIRContext.h`. I think if this complexity could somehow be abstracted away without affecting the `MLIRContext`'s public methods then it minimises its impact.

As @gysit has mentioned, I think the tradeoff here is we either use a lock around the allocator and accept lock contention cost but avoid complexity surrounding toggling thread-local storage, or, we accept this complexity in favour of avoiding lock contention overhead. My opinion is that if we choose to drop using thread-local storage for `DistinctAttributeStorage`, the performance impact on MLIR overall will be minimal as it only affects the storage allocation for this _one_ attribute. I don't foresee a situation where allocating `DistinctAttributeStorage` suddenly becomes a performance bottleneck due to there being a lock versus thread-local storage.

https://github.com/llvm/llvm-project/pull/128566