Closed Bug 1034330 Opened 6 years ago Closed 6 years ago

OdinMonkey: various cleanups

Categories

(Core :: JavaScript Engine: JIT, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(6 files, 1 obsolete file)

I've accumulated a stack of these while working on bigger patches.
Attached patch nicer-oomSplinter Review
This patch checks OOM closer to the source, propagating via enoughMemory_.  It also avoids some redundant CodeOffsetLabel -> uint32 -> CodeOffsetLabel silliness.
Attachment #8450571 - Flags: review?(benj)
Attached patch tidy-global-access (obsolete) — Splinter Review
Err, *this* is the patch that avoids the CodeOffsetLabel silliness.
Attachment #8450574 - Flags: review?(benj)
There is a lingering MaybeRetAddr in GenerateOOLConvert that is no longer necessary now that we're using exitFP.
Attachment #8450575 - Flags: review?(dtc-moz)
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.
Attachment #8450585 - Flags: review?(dtc-moz)
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.
Attachment #8450587 - Flags: review?(benj)
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.
Attachment #8450589 - Flags: review?(benj)
Now a patch that builds on x86.
Attachment #8450574 - Attachment is obsolete: true
Attachment #8450574 - Flags: review?(benj)
Attachment #8450593 - Flags: review?(benj)
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());
Attachment #8450587 - Flags: review?(benj) → review+
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?
Attachment #8450589 - Flags: review?(benj) → review+
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?
Attachment #8450593 - Flags: review?(benj) → review+
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.
Attachment #8450571 - Flags: review?(benj) → review+
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.
Attachment #8450585 - Flags: review?(dtc-moz) → review+
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?
Attachment #8450575 - Flags: review?(dtc-moz) → review+
(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".
You need to log in before you can comment on or make changes to this bug.