Closed Bug 835003 Opened 9 years ago Closed 9 years ago

simplify generation of MTableSwitch jump tables

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(2 files)

Attached patch WIPSplinter Review
The goal of this bug is to change how jump tables are filled in to avoid the DeferredJumpTable mechanism.  DeferredJumpTable causes a problem for OdinMonkey by holding onto a MTableSwitch node until after codegen is complete.  (OdinMonkey wants to generate a sequence of functions into a single MacroAssembler so, by the time we *would* process the DeferredJumpTables, the MIR nodes are long gone.  The resulting patch seems altogether simpler, reusing the existing CodeLabel machinery.

The only oddball is ARM which of course does things differently and doesn't generally support AbsoluteLabel/Bind.  First I need some feedback from Marty if I'm doing it the right way or if there is an easier way using some other existing ARM machinery.
Attachment #706726 - Flags: feedback?(mrosenberg)
Comment on attachment 706726 [details] [diff] [review]
WIP

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

Oh man, I am rather happy to se deferreddata go.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +757,5 @@
> +Assembler::writeCodePointer(AbsoluteLabel *absoluteLabel) {
> +    JS_ASSERT(!absoluteLabel->bound());
> +    // Thread the patch list through the unpatched address word in the
> +    // instruction stream.
> +    BufferOffset off = writeInst(-1);

I'm a bit nervous about -1 being in the instruction stream, particularly with a comment about unpatcheed address word", since you generally don't get a full word for addresses.
Attachment #706726 - Flags: feedback?(mrosenberg) → feedback+
Oops, that comment is out of date.  In x86-shared, we weave a linked list of uses of an AbsoluteLabel through the code buffer (which gets replaced by the address of the AbsoluteLabel when the AbsoluteLabel gets linked).  In ARM, there is no need for that extra functionality, so I just write -1 as a canary value.
Attached patch patchSplinter Review
Attachment #708194 - Flags: review?(dvander)
Comment on attachment 708194 [details] [diff] [review]
patch

Flipping r? to Hannes since he's more familiar with this code.
Attachment #708194 - Flags: review?(dvander) → review?(hv1989)
Comment on attachment 708194 [details] [diff] [review]
patch

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

Internship stuff :D. Glad we can use a CodeLabel again. That was the original way we did it, but got adjusted to DefferedDataLabel in bug 677636 as that was the only way to do this properly without putting the table in the code. That's also the reason the CodeLabel was implemented in x86/x64, but never on arm. Looks much better this way.

If masm.writeCodePointer(cl->dest()); can get moved to the visitOutOfLineTableSwitch than we can actually remove the distinction between ARM/x86/x64 for the ool path and add the ool path in CodeGenerator::visitTableSwitch. Would be cool, but it's definitely not required.

::: js/src/assembler/assembler/X86Assembler.h
@@ +2453,5 @@
>      // within the AssemblerBuffer, and code being patched by the patch buffer.  Once
>      // code has been finalized it is (platform support permitting) within a non-
>      // writable region of memory; to modify the code in an execute-only execuable
>      // pool the 'repatch' and 'relink' methods should be used.
>      

While you're at it, can you remove this white-space?

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +968,5 @@
> +        CodeLabel *cl = new CodeLabel();
> +        masm.writeCodePointer(cl->dest());
> +        if (!ool->addCodeLabel(cl))
> +            return false;
> +    }

Probably my lack on knowledge about ARM, but why do we write the codePointers not in the ool path like in x86/x64?
Attachment #708194 - Flags: review?(hv1989) → review+
> ::: js/src/ion/arm/CodeGenerator-arm.cpp
> @@ +968,5 @@
> > +        CodeLabel *cl = new CodeLabel();
> > +        masm.writeCodePointer(cl->dest());
> > +        if (!ool->addCodeLabel(cl))
> > +            return false;
> > +    }
> 
> Probably my lack on knowledge about ARM, but why do we write the
> codePointers not in the ool path like in x86/x64?

Yup.  Most of the heavy lifting is done with the single instruction 
ldr pc, [pc, r12 lsl 2]
after putting the offset into the table int r12.
Without using an extra instruction, the only way to load from a table while indexing by r12 is if the table is located immediately after the program counter, so the table has to be inline.
Hannes: based on comment 6, would you be satisfied to leave the ARM path the way it is?
Yeah, in that case it is better to keep them separated ;)
https://hg.mozilla.org/mozilla-central/rev/d693f77e3166
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.