@tlively fixed it in D66166 :)

I don't have the full context/history of this feature, but it mostly looks LGTM to me.

Thank you! We also have been floating some ideas about supporting reference types as types in another address space, but haven't actually started implementing it AFAIK. I like the general direction. Let's wait for other people's comments too.

WebAssembly change LGTM.

Oh I see. Thanks!

In D65720#1615544, @tlively wrote:

I have mixed feelings about this. One one hand I understand that this file is huge and cross-cutting changes could cause a lot of manual updating work, but on the other hand each individual test was very readable and succinct. The auto-generated version would certainly be less work to maintain, but its enormous amount of irrelevant information and repetition is unfortunate. @aheejin, what do you think about this?

LGTM with nits (+ -wasm-keep-registers thing as @tlively said). Thank you!

CC @tlively

Thank you for fixing this too!

Thank you for fixing this! This is a long-known bug we haven't fixed so far, because wasm C++ frontend lowers struct returns into sret arguments in the frontend, so we don't usually encounter this bug. Just out of curiosity, how did you discover this bug? (The reporter of this bug found it while using the Rust frontend, I heard)

LGTM!

What does this return when __wasm_init_tls has not been called?

Why should type_index symbols be unnamed, i.e., why does WasmObjectWriter try to ensure that it is unnamed? Can we possibly remove that constraints?

Where should we call __wasm_init_tls, in case within a library?

• Not sure what this means. The CL description says it tries to compile things into local-exec, but what this CL does look like checking if a global var is local-exec and if not report an error.
• What is supposed to happen when the OS is not emscripten? If we don't support TLS in non-emscripten OSes regardless of its mode (local-exec or not), should we print an error in that case too?

regcopy.mir -> reg-copy.mir

Cancel renaming

reg_instr.mir -> reg-instr.mir

LGTM

LGTM. How about adding overriden functions that takes MachineInstr argument that calls these functions? And if this is pure refactoring adding (NFC) to the title might be better.

Nice!
Then where should we call __wasm_init_tls, in case within a library?

CLANG_LINK_CLANG_DYLIB=ON does not seem to work with LLVM_INSTALL_TOOLCHAIN_ONLY=ON. I filed a bug report: https://bugs.llvm.org/show_bug.cgi?id=42575

CLANG_LINK_CLANG_DYLIB=ON does not seem to work with LLVM_INSTALL_TOOLCHAIN_ONLY=ON. I filed a bug report: https://bugs.llvm.org/show_bug.cgi?id=42575

• Print an error message instead

I thought about it, and maybe it would be safer to be explicit that we don't support clearing caches after all. I don't have strong opinions though.

In D64322#1576662, @dschuff wrote:

Another view would be that we accept that this probably doesn't get used in code that would be useful in wasm, but maybe it's in a file that has other useful code, so this would basically be a convenience feature that allows users to have fewer ifdefs in their files (even with the assumption that they aren't going to be calling this code). Someone did, after all, actually run into this problem. Speaking of, maybe we should ask them what their use case is, why they even want to compile this code?

In D64322#1573411, @sunfish wrote:

Any code that thinks it needs to "clear the cache" is very likely doing something which won't actually work on wasm. Would it be reasonable to issue a report_fatal_error here?

Yes it is what x86 does too. And it looks like they do it on purpose. From the llvm.clear_cache intrinsic semantics description:
"On platforms with coherent instruction and data caches (e.g. x86), this intrinsic is a nop. On platforms with non-coherent instruction and data cache (e.g. ARM, MIPS), the intrinsic is lowered either to appropriate instructions or a system call, if cache flushing requires special privileges. The default behavior is to emit a call to __clear_cache from the run time library."

• sret argument should be the first one

• Use llvm::any_of

• Comment fix

Could you clang-format? There seem to be lines that exceed 80 cols.

This is great!

In D64117#1567697, @aheejin wrote:

Hmm right, every other function has hidden... while you're on a roll, can you remove all hiddens here? :) Thanks!

@tlively Just wanted to check if this is OK for us.