Thank you for the pointers, I just finished making these changes. In the end, I was able to keep the CMakeLists.txt for mlir-translate unmodified because the add_mlir_translation_library call registers the library into the MLIR_TRANSLATION_LIBS variable that's already used by mlir-translate to find libraries to link to.

Thanks for adding the test!

The change looks good to me!

Avoid using the walker.

Forgot to accept :)

Thanks for iteration. I think simplicity wins here!

Looking forward to use this!

Very nice improvement, we are getting there!

LGTM

Amazing progress!

Thanks for fixing this bug!

Thanks for the explanation. Yeah then reordering the translation steps may make sense in this case!

The issue is that not all gpu.binary ops have to decay into simple strings, they could decay into a string and function stubs in a case of a CUDA like translation. Thus to avoid any potential issues with other kinds of translations it's better to always translate first gpu.binary.

Can you explain why the gpu.binary operation needs to be translated before the gpu.launch? Couldn't the gpu.launch conversion introduce an empty global variable that then is finalized once you translate the gpu.binary operation? The latter could then use getOrInsertGlobal to get and modify the global introduced during the conversion of the gpu.launch. This would make the translation robust against changing the order in which things are converted.

Rebase.

Looks even more LGTM!

Can you maybe add a test case that exercises the memory intrinsics?

LGTM modulo minor comment.

Very nice!

Address comments.

Very nice simplification!

LGTM module nit suggestion.

Very nice! Attributes make things a lot simpler.

This problem has been solved differently https://reviews.llvm.org/D155285.

Thanks LGTM!

LGTM module last nit comments.

Nice improvement!

LGTM!

Rebase and improve comments.

Very nice!

Thanks LGTM modulo ultra nit.

LGTM!

LGTM!

LGTM!

Can you add a test for the pattern or is the result always consumed by a follow up pattern?

Nice LGTM!

In D153360#4472092, @rriddle wrote:

This patch looks like it's missing bytecode support, have you tested that case? Failure there will be a miscompile, not a hard error.

Nice improvement!

Thanks for the improvement. LGTM!

The logic looks good. I have added a few nit comments.

LGTM!

Are there any more comments? @rriddle @Mogball @mehdi_amini maybe?

LGTM!

LGTM!

Can you add a small roundtrip test in llvm-project/mlir/test/Dialect/LLVMIR/func.mlir (e.g. right above the comdat stuff)?

Thanks LGTM!

Address comments.

Nice Thanks!

@mehdi_amini Thanks for the review comments. ThreadLocalCache indeed already implements a thread local storage with a non-static lifetime. I changed my code to use it.

Address comments and rebase.

Thanks LGTM!

Address comment and rebase.

LGTM!