[Mlir-commits] [mlir] [MLIR] Fix use-after-free in DistinctAttr when using crash recovery (PR #148666)

[Artemiy Bulavin]

abulavin wrote:

Hi both, thanks for your quick input on this.

> I wonder if it would make sense to abandon ThreadLocalCache in favor of a simple lock based solution. That would significantly simplify the solution and it would be much easier to understand correctness. Should there be a performance problem we can still follow up at some point.

A lock based solution is definitely much simpler IMO. In theory, a lock based solution pays the price of synchronisation overhead, in exchange for `DistinctAttr` storage persisting beyond the lifetime of the MLIR thread pool (which as you've both pointed out, isn't always the same as the lifetime of the Context) and for being more readable. In reality, I don't think that the creation of `DistinctAttr` will become a massive bottleneck in any representative MLIR compiler workload. So while lock-free access is _nice_, I think it's better to steer the tradeoff in favour of correctness.

My last commit reverts to just a single, synchronized allocator. I will update the PR description to better reflect the changes once we're all agreed. Let me know what you think.

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