Closed Bug 1728549 Opened 3 years ago Closed 3 years ago

Post-refactoring cleanup of baseline compiler

Categories

(Core :: JavaScript: WebAssembly, enhancement, P3)

Product:
Core
Component:
JavaScript: WebAssembly
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(9 files)

Bug 1728549 - Un-inline some stack map generator functions. r?jseward
48 bytes, text/x-phabricator-request
Details | Review
Bug 1728549 - Remove some platform dependencies in WasmBCMemory. r?jseward
48 bytes, text/x-phabricator-request
Details | Review
Bug 1728549 - Remove WasmBCMemory.h. r?jseward
48 bytes, text/x-phabricator-request
Details | Review
Bug 1728549 - Rearrange WasmBCMemory.cpp. r?jseward
48 bytes, text/x-phabricator-request
Details | Review
Bug 1728549 - Refactor atomic operations to make them maintainable. r?jseward
48 bytes, text/x-phabricator-request
Details | Review
Bug 1728549 - Final post-refactor reorg and cleanup. r?jseward
48 bytes, text/x-phabricator-request
Details | Review
Bug 1728549 - Make pointer values RegPtr. r?jseward
48 bytes, text/x-phabricator-request
Details | Review
Bug 1728549 - Add a 'none' case for some atomic ops. r?jseward
48 bytes, text/x-phabricator-request
Details | Review
Bug 1728549 - Make the 'none' case narrower, only wasi is affected. r?jseward CLOSED TREE
48 bytes, text/x-phabricator-request
Details | Review

Some things were intentionally delayed and some were unintentionally forgotten; let's clean them up (I expect multiple patches here).

Not moving createStackMap during the initial refactor was an
oversight. We take the opportunity to do some minor related cleanup.

Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c99e54438e7
Un-inline some stack map generator functions. r=jseward

Remove platform dependencies that depend only on whether the platform
has a HeapReg or not, by introducing a properly named #define for this
and rewriting the code slightly.

Introduce a RegIntptr alias to express integer-representing-pointer
appropriately on different platforms.

Introduce a templated move() method for register-to-register moves.

Drive-by cleanup:

Replace the overloaded free() methods for freeing registers with a
templated variant to match what we have for push, pop, need, and move.

Document the internal #defines that control the compiler.

Depends on D124446

WasmBCMemory.h held a class that was visible in WasmBCClass.h, but
this class can be internal to WasmBCMemory.cpp if some of the methods
on BaseCompiler are turned into static functions in the latter file.

Depends on D124642

No functional change, just grouping methods according to their area of
operation and adding separators to clarify.

Depends on D124643

This is a fairly large refactor without any change intended in
functionality or even (functionality-neutral) code generation.

Background:

Previously, atomic operations were split into several parts that lived
in different parts of WasmBaselineCompile.cpp and WasmBCMemory.cpp:

  • the "emitters" in WasmBaselineCompile.cpp which read bytecode and
    called code generation functions in WasmBCMemory.cpp;

  • the code generation drivers in WasmBCMemory.cpp such as atomicXchg();

  • the "poppers", which were RAII wrappers that would know about how to
    do register allocation and assignment per-platform;

  • a helper class for managing temp registers separately from the
    poppers, defined in WasmBCMemory.h; and

  • template functions that would drive the macroassembler given the
    inputs from the above.

Historically these were scattered far apart in WasmBaselineCompile.cpp
because of the layering of that file; the recent refactor just moved
some of them to WasmBCMemory.cpp without doing anything more.

This patch changes the structure as follows:

  • the "emitters" stay where they are because they are clean and
    because separating out the emitters is a good pattern we want to
    continue to use; and

  • the other parts have been brought together in WasmBCMemory.cpp and
    ifdeffed in a way that keeps related code together.

In particular, the code generation drivers now have a common form,
where it explicitly calls pop/alloc, codegen, and dealloc methods, and
the methods for an operation on a platform are defined together. This
should make code significantly more maintainable, and it reduces the
ifdeffery in the file. Also it allows the file to be organized
better.

Depends on D124644

This gets rid of some more platform ifdefs and reorganizes the functions and comments
to more naturally reflect their functional group. No functional changes.

Depends on D124645

Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3e9c2ebd825
Remove some platform dependencies in WasmBCMemory. r=jseward
https://hg.mozilla.org/integration/autoland/rev/e72165b3f9e2
Remove WasmBCMemory.h. r=jseward
https://hg.mozilla.org/integration/autoland/rev/ff900e9ba3ff
Rearrange WasmBCMemory.cpp. r=jseward
https://hg.mozilla.org/integration/autoland/rev/ca7fabfacf78
Refactor atomic operations to make them maintainable. r=jseward
https://hg.mozilla.org/integration/autoland/rev/dad808334868
Final post-refactor reorg and cleanup. r=jseward

Traditionally pointer values (tls, heap pointer) were represented as
RegI32 because we "know" that that is really a GPR, even if the GPR is
64-bit on 64-bit systems.

Later, we introduced RegPtr to hold pointer values that were not
reference values, but used only in some cases. Let's use RegPtr to
hold the tls and heapreg pointer values now, introducing better type
safety that will aid us in the memory64 work.

There's a wrinkle because RegPtr can't be stored on the value stack.
Recently a "RegIntptr" abstraction was introduced as part of the
cleanup of WasmBCMemory, but this is not necessary at this time: all
we need is a local conversion to RegI32 or RegI64 as appropriate for
the platform, in the one place where this is needed, to push a RegPtr
value. So simplify this code by removing RegIntptr again and adding
some local conversion functions.

This is needed for WASI, which has a weird configuration not previously tested.

Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3cb1ad79d239
Add a 'none' case for some atomic ops. r=jseward
Attachment #9240270 - Attachment description: Bug 1728549 - Make the 'none' case narrower, only wasi is affected. r?jseward → Bug 1728549 - Make the 'none' case narrower, only wasi is affected. r?jseward CLOSED TREE
Pushed by ctuns@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e6db6c31531
Make the 'none' case narrower, only wasi is affected. r=jseward CLOSED TREE
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f41a5f974305
Make the 'none' case narrower, only wasi is affected. r=jseward CLOSED TREE

Backed out for causing spidermonkey bustages.

Flags: needinfo?(lhansen)

Backout was resolved after some tweaking.

Flags: needinfo?(lhansen)
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41a6e728e0be
Make pointer values RegPtr. r=jseward

Backed out for causing spidermonkey failures on WasmBCMemory.cpp

Flags: needinfo?(lhansen)

There was a bug here (misspelling a function name) that CI did not find; good to get that fixed. CI caused a backout for a non-bug, not ifdeffing a static inline function that was unused in the tested configuration.

Flags: needinfo?(lhansen)
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c67ff8ccba06
Make pointer values RegPtr. r=jseward

There may be more to do but for the purposes of unblocking memory64 we're done for now.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: