1447261 - wasm: a few small Baseline compiler refactorings

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.mandatory.args.patch

It's a lie that ModuleEnvironment arguments are optional: we always pass most of them, except for the last one. This patches enforces this.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.remove.unnecessary.members.patch

mode_ and debugEnabled_ are just copied from the environment, which doesn't change during compilation, so we can just use these from the env instead of copying them.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.misc-style-tweaks.patch

And now after these two small and very scoped patches, a patch that tweaks many places in the baseline compiler, including and limited to:

- use using instead of typedef
- add trailing _ to indicate membership in a few structs
- make invalid StackHeight with a dedicated function, instead of relying on callers providing the right value
- create Stk entities immutably, removing the need for the None kind.
- add spaces here and there between operands of subtract
- remove BranchState::NoPopType

Will try this patch first; even though x64 wasm tests passed here, I'm a bit dubious that the Stk ctor thing will not cause implicit conversion issues (float32 vs float64, int32 vs int64).

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8960507 [details] [diff] [review]
1.mandatory.args.patch

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

Yeah, I think the optional args were holdovers from older code.

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 8960508 [details] [diff] [review]
2.remove.unnecessary.members.patch

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

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +9008,5 @@
>          OpBytes op;
>          CHECK(iter_.readOp(&op));
>  
> +        // When env_.debugEnabled(), every operator has breakpoint site but Op::End.
> +        if (env_.debugEnabled() && op.b0 != (uint16_t)Op::End) {

Adding an indirection here is not fabulous but if I am really concerned about that I should make emitBody() a templated function to avoid the overhead altogether, so ... OK, I guess.  I already have a work item filed anyway to look into the loop overhead in this function.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8960509 [details] [diff] [review]
3.misc-style-tweaks.patch

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

We need to talk about these changes; I don't think I want to take this patch.

The changes in expression spacing are technically bug 1333020 although no harm done landing them here, of course.

"using" is not generally an improvement over "typedef" so why the change?

As for some of the naming changes: "masm" is not "masm_" because the risk of confusion is zero and I want less typing, not more; ditto "ra" is not "ra_", and other cases (knownGPR, knownFPU).

The changes to push() can only land after performance measurements; we need to make sure that the infallibleEmplace is inlined all the way.  It *should* be, but we should make sure.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ -5401,5 @@
>      // To disable this optimization it is enough always to return false from
>      // sniffConditionalControl{Cmp,Eqz}.
>  
>      struct BranchState {
> -        enum NoPopType {

Don't remove this.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Lars T Hansen [:lth] from comment #6)
> Comment on attachment 8960509 [details] [diff] [review]
> 3.misc-style-tweaks.patch
> 
> Review of attachment 8960509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We need to talk about these changes; I don't think I want to take this patch.

Sure, I can remove some parts if you feel strongly about them.

> The changes in expression spacing are technically bug 1333020 although no
> harm done landing them here, of course.

Hah, since I didn't see as many of them as before, I thought they were almost all fixed and just fixed the ones I've seen. Happy to split out in a patch in the other bug, if you prefer.

> "using" is not generally an improvement over "typedef" so why the change?

It's just cleaner, more recent, and in my opinion more expressive too, by following the rather common principle of "assignment goes to the left", so I read them more like: x := type. Even after a decade of C++ I still tend to mix up the order in typedef declarations, or to forget parenthesis when typedefing a function. I've started using `using` in more places in js/wasm/ and wanted to convert all the typedefs at some point there. It has no impact on performance but makes reading code easier, I find.

> As for some of the naming changes: "masm" is not "masm_" because the risk of
> confusion is zero and I want less typing, not more; ditto "ra" is not "ra_",
> and other cases (knownGPR, knownFPU).

Doh :( This is just plain inconsistent when there are mixed variables with trailing underscore and some others without it. I can live with it, of course, but it'd seem more consistent with the rest of the wasm sub-directory. Editor auto-completion also makes more typing a non-issue in my experience. I'm happy to drop the change for the struct which had no trailing underscores at all and for which I replaced them; but there was one struct which had some members with trailing and some without, which is just confusing. Does it sound like a good tradeoff?

> The changes to push() can only land after performance measurements; we need
> to make sure that the infallibleEmplace is inlined all the way.  It *should*
> be, but we should make sure.

Fair enough, will check.

> ::: js/src/wasm/WasmBaselineCompile.cpp
> @@ -5401,5 @@
> >      // To disable this optimization it is enough always to return false from
> >      // sniffConditionalControl{Cmp,Eqz}.
> >  
> >      struct BranchState {
> > -        enum NoPopType {
> 
> Don't remove this.

Why not? There's no confusion with other ctors, and the first ctor that takes only a Label* also should have had NoPopType in this case, since it also doesn't pop the stack.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Checked Tanks and AngryBots wasm compilation: --no-threads --no-wasm-ion doesn't get any slower (evens slightly faster for AngryBots, but that might just be noise).

Comment 9

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> (In reply to Lars T Hansen [:lth] from comment #6)
> > Comment on attachment 8960509 [details] [diff] [review]
> > 3.misc-style-tweaks.patch
> > 
> > Review of attachment 8960509 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We need to talk about these changes; I don't think I want to take this patch.
> 
> Sure, I can remove some parts if you feel strongly about them.
> 
> > The changes in expression spacing are technically bug 1333020 although no
> > harm done landing them here, of course.
> 
> Hah, since I didn't see as many of them as before, I thought they were
> almost all fixed and just fixed the ones I've seen. Happy to split out in a
> patch in the other bug, if you prefer.

No, just land whatever's ready.  At some point we'll just close the other bug.

> > "using" is not generally an improvement over "typedef" so why the change?
> 
> It's just cleaner, more recent, and in my opinion more expressive too, by
> following the rather common principle of "assignment goes to the left", so I
> read them more like: x := type. Even after a decade of C++ I still tend to
> mix up the order in typedef declarations, or to forget parenthesis when
> typedefing a function. I've started using `using` in more places in js/wasm/
> and wanted to convert all the typedefs at some point there. It has no impact
> on performance but makes reading code easier, I find.

OK.  I guess I don't really care :)  Be my guest.

> > As for some of the naming changes: "masm" is not "masm_" because the risk of
> > confusion is zero and I want less typing, not more; ditto "ra" is not "ra_",
> > and other cases (knownGPR, knownFPU).
> 
> Doh :( This is just plain inconsistent when there are mixed variables with
> trailing underscore and some others without it. I can live with it, of
> course, but it'd seem more consistent with the rest of the wasm
> sub-directory.

Well, consistent...  'masm' appears on 1253 lines in wasm/ (not including those that have 'masm_'), 'masm_' on 46 (all in WasmGenerator.cpp).  I would like to argue for a general exception for the identifier "masm" - it is always the macro-assembler.

By the same rule, 'fr' (a stupid name, but short) and 'ra' are exempt from the underscore in BaseCompiler, and I would prefer that they remain exempt in other classes that reference them for the same reason.  I won't put up a fight about any other names.

I see that 'specific' and the join regs are also underscore-less in BaseCompiler; there's no good reason for that except history, so they could be fixed, if you're so inclined.

> Editor auto-completion also makes more typing a non-issue in
> my experience.

Yeah, I probably should move into that future but I probably won't, so I'm stuck typing everything.

> I'm happy to drop the change for the struct which had no
> trailing underscores at all and for which I replaced them; but there was one
> struct which had some members with trailing and some without, which is just
> confusing. Does it sound like a good tradeoff?

As I said, I'd prefer 'masm', 'ra', (and to the extent it's relevant though I suspect it isn't) 'fr' to be exempt globally, and for other members I'm fine with a rule that calls for underscore.

> > ::: js/src/wasm/WasmBaselineCompile.cpp
> > @@ -5401,5 @@
> > >      // To disable this optimization it is enough always to return false from
> > >      // sniffConditionalControl{Cmp,Eqz}.
> > >  
> > >      struct BranchState {
> > > -        enum NoPopType {
> > 
> > Don't remove this.
> 
> Why not? There's no confusion with other ctors, and the first ctor that
> takes only a Label* also should have had NoPopType in this case, since it
> also doesn't pop the stack.

Hm.  Historically there may have been ambiguity here, because StackHeight was a number and some of these arguments were optional, this code has changed quite a bit recently.  I see you also fixed the invertBranch to be bool, I wonder how that started out as uint32_t.

Fair point - this can go.

Looks like you inserted some beginning-of-line spaces on three blank lines in the definition of Stk.

Comment 10

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> Checked Tanks and AngryBots wasm compilation: --no-threads --no-wasm-ion
> doesn't get any slower (evens slightly faster for AngryBots, but that might
> just be noise).

You probably meant Tanks and ZenGarden?  (If you have an AngryBots that still compiles and runs, I want it...)

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Lars T Hansen [:lth] from comment #10)
> You probably meant Tanks and ZenGarden?  (If you have an AngryBots that
> still compiles and runs, I want it...)

For history, really meant AngryBots here, and forwarded to Lars.

Also, as seen on IRC:

> lth| i'm not really here but i just wanted to note rs=me for the refactorings patch if you agree to leave "masm" as "masm" and not "masm_" and ditto for "ra".  in case you want to be done with that patch.

Let's do this!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e287da9ee9f0
Make most arguments mandatory in wasm::ModuleEnvironment ctor; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0de96cd547
Remove debugEnabled/mode from the wasm::BaselineCompile ctor; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e267eba38256
Better code style and tweaks to the wasm baseline compiler; r=lth

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e287da9ee9f0
https://hg.mozilla.org/mozilla-central/rev/2a0de96cd547
https://hg.mozilla.org/mozilla-central/rev/e267eba38256