1697371 - Templatize most/all vanilla baseline emitters

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

Starting with SIMD, many emitters in the baseline compiler have been templatized so that the tedium of pushing and popping values and managing registers and temps has been reduced to a bare minimum, being handled centrally in a group of templatized functions. For SIMD this was essential, since there were so many operations. But we can apply the same mechanism to existing "computational" instruction emitters, further removing clutter.

The cost of the templatization is possibly one function call per operation - almost certainly affordable. We'll want to verify that we don't regress compilation performance and that the emitted code looks reasonable.

Comment 1

[Lars T Hansen [:lth]]

Attachment: templatize.patch

Sketch. Too few operations have been rewritten here to make a meaningful difference, but there are many of them that can be handled, both binary and unary.

(This is not expected to compile, I need a templatized version of popConst, at a minimum.)

Removing this "simple" clutter doesn't reduce complexity very much, but I'm hoping it will trigger ideas for handling more complex cases that have platform-dependent needs for temps, for example. That worked out well for SIMD.

Comment 2

[Lars T Hansen [:lth]]

Another benefit from this type of work is that it may allow the baseline compiler to be factored more easily: the functions that represent the core of the operations may be moved to a different file or moved into an unobtrusive corner of the current file. As the baseline compiler is currently 16K lines where the complex and the mundane are intermingled, either solution would be progress...

Comment 3

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - Templatize baseline compiler part 2: Easy cases. r?rhunt

Make the templatized SIMD emitters available for common use and expand
the set a little. Translate some existing binop emitters into this
style.

(Many more should be translated before this patch is ready.)

Comment 4

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - Templatize baseline compiler part 1: Move stuff around and rename. r?rhunt

The templates that were introduced for SIMD are promoted from
emitVectorWhatever to emitWhatever, so that they can be used more
generally. They are also moved away from SIMD-specific code to a more
generic place in the compiler.

Some type-specific names for which type information is otherwise
available though a parameter type are changed to become type-agnostic
(popConstT -> popConst for example), this will aid further template
use.

Comment 5

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - Templatize baseline compiler part 3: Popcnt. r?rhunt

Popcount may need a temp; whether it does depends on both the platform
and the CPU. A custom temp allocator function can be passed to the
emitter and used generically.

Depends on D107798

Comment 6

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - Templatize baseline compiler part 4: Shifts and 32-bit rotates. r?rhunt

Shifts and rotates may require a variable RHS to be in a specific
register. We can express this generically by passing an rhs-popper
function to the emitter.

(64-bit rotates are additionally complicated by requiring a temp, and
are not translated in this patch.)

Depends on D109609

Comment 7

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - Templatize baseline compiler part 5: eqz and wrap. r?rhunt

Depends on D109610

Comment 8

[Lars T Hansen [:lth]]

Updated patches deal with some subtle issues where the baseline compiler uses its own temp registers instead of the macroassembler's.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85fe1b348664
Templatize baseline compiler part 1: Move stuff around and rename. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/8869b6a4843e
Templatize baseline compiler part 2: Easy cases. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/4b683260bf2f
Templatize baseline compiler part 3: Popcnt. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/c6a95e1a6b94
Templatize baseline compiler part 4: Shifts and 32-bit rotates. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/6f12cc06efc0
Templatize baseline compiler part 5: eqz and wrap. r=rhunt

Comment 10

[Cristian Brindusan [:cbrindusan]]

Backed out 5 changesets (Bug 1697371) for causing sm bustages in WasmBaselineCompile.cpp.
https://hg.mozilla.org/integration/autoland/rev/11c062bfaa5334bf7e8f9142692f05c455539e16

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=6f12cc06efc005daf9da88ae78d39d48506d21b7&selectedTaskRun=LpuW7j5GQOeF6S6SNiUD5A.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=337280857&repo=autoland&lineNumber=748

Comment 11

[Lars T Hansen [:lth]]

Compiler issue: C++ is not one programming language. Template specializations cannot always appear inside a class body.

Comment 12

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - Templatize baseline compiler part 6: simple instance call ops. r?rhunt

Depends on D109611

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c24133b2b6d
Templatize baseline compiler part 1: Move stuff around and rename. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/ad98cd5334b3
Templatize baseline compiler part 2: Easy cases. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/2da4589bf1e2
Templatize baseline compiler part 3: Popcnt. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/1992090d7578
Templatize baseline compiler part 4: Shifts and 32-bit rotates. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/b070428c3e07
Templatize baseline compiler part 5: eqz and wrap. r=rhunt

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/9c24133b2b6d
https://hg.mozilla.org/mozilla-central/rev/ad98cd5334b3
https://hg.mozilla.org/mozilla-central/rev/2da4589bf1e2
https://hg.mozilla.org/mozilla-central/rev/1992090d7578
https://hg.mozilla.org/mozilla-central/rev/b070428c3e07

Comment 15

[Lars T Hansen [:lth]]

Comment on attachment 9217415 [details]
Bug 1697371 - Templatize baseline compiler part 6: simple instance call ops. r?rhunt

I'm conflicted about this one. On the one hand, it is nice to clean up all those ops that are implemented as pure instance calls by abstracting out all the bookkeeping of passing args and handling results - less room for error. On the other hand, we add three definitions of emitInstanceCallOp to handle all the cases, and none of the cases where an instance call is just one of several possibilities are handled, nor are cases that are always an instance call but with some preprocessing / conditions, like emitWait and emitWake. Handling them would seem to require additional variants of emitInstanceCallOp.

(I now see I could handle TableCopy in the present patch, it's just an oversight that I don't.)

Worth it, or too much of a good thing?

Comment 16

[Ryan Hunt [:rhunt]]

Comment on attachment 9217415 [details]
Bug 1697371 - Templatize baseline compiler part 6: simple instance call ops. r?rhunt

I left my feedback on the revision.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18b241ffe60d
Templatize baseline compiler part 6: simple instance call ops. r=rhunt

Comment 18

[Lars T Hansen [:lth]]

It is possible to do more for the individual emitter functions, along several dimensions:

• code generators that take one path for some constant rhs and another path for others (quite a few of these)
• code generators that have custom poppers (so as to target specific registers) (many of these)
• code generators that need to reserve specific registers because they will be clobbered by codegen on the platform (mul and div) or are used as specific temps (rotate)

Increasingly the machinery becomes specialized (and thus pays off less) and there's a growing chance of conflicts among the templates; the latter can be mitigated with typed wrappers or cookies to avoid confusion (I'll attach a suggestion), but the increasing specialization hurts more.

Also, none of the above get at the two sources of hard complexity: platform ifdefs, and dealing with control flow. They just mitigate the hardship of managing the stack some.

For platform ifdefs, it's possible that some complexity could be hidden by factoring into platform-specific files and creating better boundaries (atomic ops and memory ops are obvious but there are many of these) and this would simplify understanding, if not exactly maintenance.

I'll keep this open for now but I'm not sure exactly where to take it next.

Comment 19

[Lars T Hansen [:lth]]

Attachment: type-specific-wrapper.patch

Example of a type specific wrapper that allows template matching confusion to be avoided.

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/18b241ffe60d

Comment 21

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - remove unnecessary platform ifdefs. r?yury

Some ifdefs in the dispatch code can be removed by adding a second case higher up
where the ifdefs are expected.

Comment 22

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - clean up platform ifdefs for bitselect. r?yury

This makes more code common and moves platform code to a location where it is expected.

Depends on D119085

Comment 23

[Lars T Hansen [:lth]]

Attachment: Bug 1697371 - Clean up and simplify right shift i64x2. r?yury

i64x2 signed right shift requires a custom popper on x86/x64, but
that's no excuse for the convoluted logic we had; clean this up as
much as possible. The ifdef is retained but this function is now a
possible target for common emitter logic that allows for a custom RHS
popper (a number of other methods have the same pattern).

Depends on D119090

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56b151032feb
remove unnecessary platform ifdefs. r=yury
https://hg.mozilla.org/integration/autoland/rev/92dd81639df6
clean up platform ifdefs for bitselect. r=yury
https://hg.mozilla.org/integration/autoland/rev/9798780fc717
Clean up and simplify right shift i64x2. r=yury

Comment 25

[Natalia Csoregi [:nataliaCs]]

Backed out for spidermonkey bustage on WasmBaselineCompile.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/4c2a5ad9f027181be19879ad7ac853db2ff520a6

• push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=9798780fc717909b28c70d32a88b467ff951c25c&group_state=expanded&selectedTaskRun=FQcbU-CpRsifoKuom4KVUw.0

• failure log: https://treeherder.mozilla.org/logviewer?job_id=344561555&repo=autoland&lineNumber=781

[task 2021-07-07T11:57:09.094Z]  0:47.31 clang-12: warning: argument unused during compilation: '-std=gnu99' [-Wunused-command-line-argument]
[task 2021-07-07T11:57:11.267Z]  0:49.48 memory/mozalloc
[task 2021-07-07T11:57:12.295Z]  0:50.51 mfbt
[task 2021-07-07T11:57:17.694Z]  0:55.91 In file included from Unified_cpp_js_src_wasm0.cpp:20:
[task 2021-07-07T11:57:17.694Z]  0:55.91 /builds/worker/checkouts/gecko/js/src/wasm/WasmBaselineCompile.cpp:3058:8: error: unknown type name 'RegV128'
[task 2021-07-07T11:57:17.695Z]  0:55.91 static RegV128 BitselectV128(BaseCompiler& bc, RegV128 rs1, RegV128 rs2,
[task 2021-07-07T11:57:17.696Z]  0:55.91        ^
[task 2021-07-07T11:57:17.697Z]  0:55.91 /builds/worker/checkouts/gecko/js/src/wasm/WasmBaselineCompile.cpp:3058:48: error: unknown type name 'RegV128'
[task 2021-07-07T11:57:17.698Z]  0:55.91 static RegV128 BitselectV128(BaseCompiler& bc, RegV128 rs1, RegV128 rs2,
[task 2021-07-07T11:57:17.698Z]  0:55.91                                                ^
[task 2021-07-07T11:57:17.699Z]  0:55.91 /builds/worker/checkouts/gecko/js/src/wasm/WasmBaselineCompile.cpp:3058:61: error: unknown type name 'RegV128'
[task 2021-07-07T11:57:17.700Z]  0:55.91 static RegV128 BitselectV128(BaseCompiler& bc, RegV128 rs1, RegV128 rs2,
[task 2021-07-07T11:57:17.701Z]  0:55.91                                                             ^
[task 2021-07-07T11:57:17.701Z]  0:55.91 /builds/worker/checkouts/gecko/js/src/wasm/WasmBaselineCompile.cpp:3059:30: error: unknown type name 'RegV128'
[task 2021-07-07T11:57:17.702Z]  0:55.91                              RegV128 rs3);
[task 2021-07-07T11:57:17.703Z]  0:55.91                              ^

Comment 26

[Lars T Hansen [:lth]]

Missing #ifdef.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/296bacb551df
remove unnecessary platform ifdefs. r=yury
https://hg.mozilla.org/integration/autoland/rev/9089f33f4406
clean up platform ifdefs for bitselect. r=yury
https://hg.mozilla.org/integration/autoland/rev/8f2ef8160c83
Clean up and simplify right shift i64x2. r=yury

Comment 28

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/296bacb551df
https://hg.mozilla.org/mozilla-central/rev/9089f33f4406
https://hg.mozilla.org/mozilla-central/rev/8f2ef8160c83

Comment 29

[Lars T Hansen [:lth]]

I think this is "done" for now. There are other changes possible / pending but they don't fit in neatly with a "templatize" type of theme.