1311433 - Wasm baseline: clean up call abstractions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Lars T Hansen [:lth]]

The function calling code in the wasm baseline compiler looks like Belgium at the end of any major war - vast armies have trampled through it in their haste to get somewhere else, dug in to fight when necessary, and generally ravaged it.  As a result it is now buggy, has many duplicated paths, and generates inefficient and confusing code.  Let's fix it.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1311433-cleanup-call.patch

Cleans up some calls by commoning code and making some code paths significantly less ad hoc, notably managing the Tls pointer and reloading the register state from Tls after some calls.

One could do more here, but this is a good first start.

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8802859 [details] [diff] [review]
bug1311433-cleanup-call.patch

Cancelling review on this because I want to clean a little more.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1311433-cleanup-call-v2.patch

Clean up the function calling logic some (see previous comments).   This version improves on the previous version by also simplifying the available call styles, removing dead code (we never need to save HeapReg and GlobalReg, they can be reloaded from Tls when they need to be reloaded) and improving naming.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1311433-cleanup-call-v3.patch

And on the subject of vast armies trampling through: Luke has been in this code again, so here's a rebased patch.  This is simpler than the previous version, because currentMemory and growMemory are no longer merged.

Again, I think there's more that can be done here, but it's still worth doing this.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8804192 [details] [diff] [review]
bug1311433-cleanup-call-v3.patch

Review of attachment 8804192 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup! Is this preparation to do bigger cleanups? Like e.g. the Belgium political structure?

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +147,5 @@
>  typedef bool HandleNaNSpecially;
> +
> +// UseABI::Wasm assumes Tls/Heap/Global are nonvolatile, except when
> +// also InterModule::True, when they must be reloaded after the call.
> +// In the baseline compiler, the Tls must be set up before the call.

UseABI::Wasm assumes Tls/Heap/Global registers are nonvolatile, except when InterModule::True is set. In that case it must be reloaded after the call. In the wasm baseline compiler the TLs must be set up before the call.

"it must be reloaded" or "it will get reloaded" by FunctionCall?

English is not my native language. Therefore I'm quite hesitant to forcefully request the above requested changes. Feel free only take the parts that look correct.

@@ +151,5 @@
> +// In the baseline compiler, the Tls must be set up before the call.
> +//
> +// UseABI::System requires Tls/Heap/Global to be reloaded after
> +// the call, and in addition the parameter passing mechanism may be
> +// slightly different.  But the Tls need not be set up before the call.

But the TLS doesn't need to be set up before the call.

@@ +3413,5 @@
>      void emitReinterpretI64AsF64();
> +    MOZ_MUST_USE
> +    bool emitGrowMemory();
> +    MOZ_MUST_USE
> +    bool emitCurrentMemory();

I think in most cases we don't have a newline between MOZ_MUST_USE and the function definition. I also like that more, but I don't think we have style rules for it?

Comment 6

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37dfd653f3a5ee6bc76b6d41e32621870ca95d7e
Bug 1311433 - Wasm baseline: clean up function calling code. r=h4writer

Comment 7

[Lars T Hansen [:lth]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> 
> Nice cleanup! Is this preparation to do bigger cleanups? Like e.g. the
> Belgium political structure?

Fixing Belgium is going to cost you extra...  I have visions of epic overtime.

Smaller cleanups of the baseline compiler are probably happening, though!

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/37dfd653f3a5