setTempRet0 and getTempRet0 are inconsistent in their implementation.

[sbc100]

There are several places there the need to some kind of temporary is need is needed / used in emscripten.

1. When returning 64-bit values to emscripten:

Under wasm this is handled in either asm2wasm or emscripten-wasm-finalize which both run the legalize-js-interface pass: https://github.com/WebAssembly/binaryen/blob/master/src/passes/LegalizeJSInterface.cpp#L256

In this case the temporary uses is a wasm global and is only ever set via inline calls to set_global in the wrapper functions. getTempRet0 in synthesized here then exported so that JS can call it. For an example of this run the other.test_sixtyfour_bit_return_value test.

2. setjmp/longjmp + exception handling in fastcmp and llvm upstream:

See lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp in both fastcmp and upstream.
Here we have a global variable in linear memory and a helper function called setTempRet0 that JS can use to set it.

So we have two different globals being used, which happens to work because JS code only need to set one of them an get the other.

This is somewhat confusing and should probably be fixed. There is more complexity that I'm also missing. Continuing to investigate. My current feeling is that the codegen in both llvm and binaryen should always use the getters and setters, and then the implementations can be up the linker/LTO.

[aheejin]

I guess we can have two different names..? tempRet0 is already pretty confusing name to begin with. It looks like tempRet0 is used in two different ways, each for exception and sjlj handling, in the LLVM backend, which followed fastcomp's convention, so we can probably name it something like EHTempRet or something... I know this is a terrible name so suggestions are welcome. And for the 64-bit value thing, we can keep the name or change that name to something that better explains the purpose if we choose to.

[sbc100]

I agree that splittin them into separate variables would solve the problem and increase comprehension. But since they are trying to do that same thing I think I'd rather unify them at this point.

My initial plan is to change the implementations to always use the set functions rather than settings these values directly. Alon, do you forsee a performance hit if change "set_global"/"store" to "call setTempRet0"? Presumably they will be inline-able by binaryen anyway?

[aheejin]

I agree that splittin them into separate variables would solve the problem and increase comprehension. But since they are trying to do that same thing I think I'd rather unify them at this point.

I don't understand this point. They are used to mean different things, and they don't even set the same thing (one sets a global variable in linear memory, and the other sets an wasm global).

[kripken]

Another option to avoid the problem (of inconsistency, using the get from one method with the set of another) may be to keep the get and set together. Specifically, we can make the extras.c get and set depend on each other (using a volatile trick perhaps), so that if one is kept alive both are. If both are alive, Binaryen's legalize pass will use them, and that should be fine.

Another option is to standardize a wasm global for this, like I believe we standardize the stack pointer.

[kripken]

Alon, do you forsee a performance hit if change "set_global"/"store" to "call setTempRet0"? Presumably they will be inline-able by binaryen anyway?

Yeah, Binaryen should inline a small getter/setter like that.

[sbc100]

I think we should revisit this.. since the solution we ended up with was to always have getTempRet0/setTempRet0 available as imports from the environment.

A better solution would avoid environmental imports completely and define these functions in native code.

[aheejin]

Why were they moved to JS in #7358? I think they used to be in native code long ago, and I'm not familiar with the history after that.

[sbc100]

Indeed, i make bunch of changes to move them to JS so there were always imported and never defined in the wasm or exported. I now think that was the wrong approach and I'm proposing to move them back inside the wasm file :)

We also now have the ability to define wasm globals in assembly which we didn't have before so we can use a wasm global to hold this.