1034330 - OdinMonkey: various cleanups

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Luke Wagner [:luke]]

I've accumulated a stack of these while working on bigger patches.

Comment 1

[Luke Wagner [:luke]]

Attachment: nicer-oom

This patch checks OOM closer to the source, propagating via enoughMemory_.  It also avoids some redundant CodeOffsetLabel -> uint32 -> CodeOffsetLabel silliness.

Comment 2

[Luke Wagner [:luke]]

Attachment: tidy-global-access

Err, *this* is the patch that avoids the CodeOffsetLabel silliness.

Comment 3

[Luke Wagner [:luke]]

Attachment: rm-lingering-MaybeRetAddr

There is a lingering MaybeRetAddr in GenerateOOLConvert that is no longer necessary now that we're using exitFP.

Comment 4

[Luke Wagner [:luke]]

Attachment: simplify-ion-exit

This patch simplifies GenerateFFIIonExit in a few ways:
 - It avoids separately pushing the heap/global registers.  Instead, they are included in the stack budget and stored into directly.  This is allows for a later patch to have a single GenerateAsmJSPrologue/Epilogue shared by all asm.js exits and functions.
 - Remove some debug-only checks that I don't think really help us here (I've never seen them catch anything and they don't seem prone to regression) to simplify/shorten the code.
 - Inline GenerateOOLConvert: I find it annoying to have this in a separate function because I constantly have to look back and forth to see how it relates to GenerateFFIonExit.  I find that merging them makes it easier to read.

Comment 5

[Luke Wagner [:luke]]

Attachment: rm-assume-unreachable

I don't know about y'all, but having assumeUnreachable call out to C++ is really annoying.  For one thing, it makes the generated code less readable (by injecting a ton of pushes/pops).  For another, when I hit this, it's annoying to work my way back up to the point of failure.

Comment 6

[Luke Wagner [:luke]]

Attachment: rename-some-things

This patch just renames some variables.  Later patches add various "offset" fields which were ambiguous with these, so this patch switches the names to explicitly contain 'src'.  Also, "Offset" doesn't seem necessary given the type is an integer, so I removed that which makes the names a bit shorter.

Comment 7

[Luke Wagner [:luke]]

Attachment: tidy-global-access

Now a patch that builds on x86.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8450587 [details] [diff] [review]
rm-assume-unreachable

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

Fun: I have in my patch queue a dirty hack that adds a variant, assumeUnreachableAsmJS(unsigned), and the unsigned gets mapped to an enum value that allows the C++ function to know which error message to show (this way, it can resist de/serialization). But it's fine not to have it at all, as well.

::: js/src/jit/IonMacroAssembler.cpp
@@ +1338,5 @@
>  void
>  MacroAssembler::assumeUnreachable(const char *output)
>  {
>  #ifdef DEBUG
> +    if (!IsCompilingAsmJS()) {

I'd also delete the users of assumeUnreachable (i think there are only 2 uses) in this case and here
JS_ASSERT(!IsCompilingAsmJS());

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8450589 [details] [diff] [review]
rename-some-things

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

I do like short names too.

::: js/src/jit/AsmJS.cpp
@@ +824,1 @@
>          bool defined_;

ooc, did you reorder these members by sizeof, as a heuristic to improve packing?

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8450593 [details] [diff] [review]
tidy-global-access

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

Haha, nice catch.

::: js/src/jit/x64/CodeGenerator-x64.cpp
@@ +368,5 @@
>      Register index = ToRegister(ins->index());
>      Register tmp = ToRegister(ins->temp());
>      Register out = ToRegister(ins->output());
>  
> +    masm.append(AsmJSGlobalAccess(masm.leaRipRelative(tmp), mir->globalDataOffset()));

nit: for symmetry with other functions, could you keep the append call after the effective codegen call?

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8450571 [details] [diff] [review]
nicer-oom

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

I was concerned that OOM errors would get reported later with this patch, but the Vector's method doesn't make the situation worse if we're already running out of storage. As it's way nicer to read, r=me.

Comment 12

[Douglas Crosher [:dougc]]

Comment on attachment 8450585 [details] [diff] [review]
simplify-ion-exit

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

Looks good, nice clean up, and it passes my regular testing.

Comment 13

[Douglas Crosher [:dougc]]

Comment on attachment 8450575 [details] [diff] [review]
rm-lingering-MaybeRetAddr

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

Looks good.

::: js/src/jit/AsmJS.cpp
@@ +6221,5 @@
>      // At the point of the call, the stack layout shall be (sp grows to the left):
> +    //   | stack args | padding | Value argv[] | padding | retaddr | caller stack args |
> +    // The padding between stack args and argv ensures that sp is aligned. The
> +    // padding between argv and retaddr ensures that argv is aligned.
> +    unsigned offsetToArgv = AlignBytes(StackArgBytes(invokeArgTypes), StackAlignment);

This was an alignment fix?

@@ +6316,5 @@
>      // The stack is assumed to be aligned.  The frame is allocated by GenerateFFIIonExit and
>      // the stack usage here needs to kept in sync with GenerateFFIIonExit.
>  
>      // Store value
> +    unsigned offsetToArgv = StackArgBytes(callArgTypes);

Noticed that the Value is aligned on the stack in GenerateFFIInterpreterExit yet alignment is not attempted here. Is that because we know there will be no stack arguments, and if so then could this be asserted?

Comment 14

[Luke Wagner [:luke]]

(In reply to Douglas Crosher [:dougc] from comment #13)
> > +    unsigned offsetToArgv = AlignBytes(StackArgBytes(invokeArgTypes), StackAlignment);
> 
> This was an alignment fix?

The change was that MaybeRetAddr was removed.

> Noticed that the Value is aligned on the stack in GenerateFFIInterpreterExit
> yet alignment is not attempted here. Is that because we know there will be
> no stack arguments, and if so then could this be asserted?

Good point, will assert.

(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> I'd also delete the users of assumeUnreachable (i think there are only 2
> uses) in this case and here
> JS_ASSERT(!IsCompilingAsmJS());

Well, the problem is that assumeUnreachable doesn't just show up in AsmJS.cpp but it can show up in random CodeGenenerator* usage.  I'd rather not add this unnecessary limitation since it'll just annoy people if they hit it.

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> ooc, did you reorder these members by sizeof, as a heuristic to improve
> packing?

Yeah, the general (not bit-field) heuristic for packing is "large-to-small".

Comment 15

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e121695598
https://hg.mozilla.org/integration/mozilla-inbound/rev/78f24480f680
https://hg.mozilla.org/integration/mozilla-inbound/rev/240771ad7e88
https://hg.mozilla.org/integration/mozilla-inbound/rev/826267db3a10
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2b3cd7ec63
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f4fe9e01d6

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d0f4fe9e01d6
https://hg.mozilla.org/mozilla-central/rev/ae2b3cd7ec63
https://hg.mozilla.org/mozilla-central/rev/826267db3a10
https://hg.mozilla.org/mozilla-central/rev/240771ad7e88
https://hg.mozilla.org/mozilla-central/rev/78f24480f680
https://hg.mozilla.org/mozilla-central/rev/b7e121695598