[PATCH] D53240: [WebAssembly] WebAssemblyLowerEmscriptenEHSjLj: use getter/setter for accessing tempRet0

Wed Oct 17 13:00:25 PDT 2018

sbc100 added a comment.

In https://reviews.llvm.org/D53240#1267875, @aheejin wrote:

> Do you think all variables used to communicate between wasm and JS use not global variables but a few predetermined wasm globals? I think that's a bit strict restriction.

No, the backing store used by setTempRet0 and getTempRet0 should independent of the users of those functions.  That is one reason for this change.  So that we can independently choose to use either wasm global or a memory location.  As it happens wasm globals are basically TLS so in this case I do think its better than a raw memory location.  Of course a TLS memory location would be find too, but that would probably be marginally more expensive because you'd need to add an offset to the TLS base.

> And I'll repeat what I said in my previous comment:
> 
>> 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). Should they share the same name in the first place?
> 
> I couldn't understand the motivation behind this change, that's all. This stemmed from my patch <https://github.com/kripken/emscripten/pull/7268> that tried to first eliminate seemingly unused `getTempRet0` and then tried to export it to be consistent with other methods, which broke another test. The reason that another test was broken was because we were using `__tempRet0` for two different purpose. So I thought if we divide the variable the problem would be solved.

I think unifying and simplifying if fine in this case.  This notion is that tempRet0 is used a TLS scratch space and I think its OK that it has several use cases.   The alternative you are suggesting is to create several sets of global+getter+setter right? getReturnHighBits/setReturnHighBits/__returnHighBit + getLongJmpArg/setLongJmpArg/__longJmpArg ... etc.   That seems lake added complexity we don't need although I can see that argument.  Even if we do decide that is better, I'd first like to solve it this way, so at least we have one unified example or how to do this, and we can split it out later if we decide that is preferable.

> Maybe I'm mistaken, so then please let me know. 😅

Repository:
  rL LLVM

https://reviews.llvm.org/D53240