Last Comment Bug 689831 - IonMonkey: Change type testing API
: IonMonkey: Change type testing API
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
:
Mentors:
Depends on:
Blocks: 689325
  Show dependency treegraph
 
Reported: 2011-09-27 19:24 PDT by David Anderson [:dvander]
Modified: 2011-10-05 23:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip v0 (11.93 KB, patch)
2011-09-27 19:24 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
patch (20.69 KB, patch)
2011-09-28 16:37 PDT, David Anderson [:dvander]
sstangl: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-09-27 19:24:09 PDT
Created attachment 562949 [details] [diff] [review]
wip v0

Marty noted that "masm.j" in platform-independent code was awkward in the context of ARM, and now adding more paths like this, I agree. I would like to shift to having a "test-and-branch" pattern like Nitro's branch32(), branchPtr(), etc, as part of the MacroAssembler API. That includes existing tests which take and return an Assembler::Condition.

Here's an example of testInt32 and testUndefined converted to the new scheme.
Comment 1 Nicolas B. Pierron [:nbp] 2011-09-28 03:42:22 PDT
(In reply to David Anderson [:dvander] from comment #0)
> Created attachment 562949 [details] [diff] [review] [diff] [details] [review]
> wip v0
> 
> Marty noted that "masm.j" in platform-independent code was awkward in the
> context of ARM, and now adding more paths like this, I agree. I would like
> to shift to having a "test-and-branch" pattern like Nitro's branch32(),
> branchPtr(), etc, as part of the MacroAssembler API. That includes existing
> tests which take and return an Assembler::Condition.
> 
> Here's an example of testInt32 and testUndefined converted to the new scheme.

(cf 680315) What about the idea of packing the test inside the function and doing the jump outside the function.  Could that also be applied on ARM?  Or the test should be moved to the caller?  Otherwise could we have a call-and-branch with a test-and-return ?
Comment 2 Sean Stangl [:sstangl] 2011-09-28 11:18:26 PDT
This change seems good. We also had branch32(), branchTest32(), and branchPtr() in JM; they worked nicely.

It would help my understanding to explain the problems that ARM faces with j(). Since these new branchTestFoo() functions seem to still use j() internally, I'm not sure how this helps ARM, but I do like the interface regardless.

> Label unknown;
> masm.branchTestUndefined(Assembler::NotEqual, operand, &unknown);
> if (!bailoutFrom(&unknown, lir->snapshot()))
>     return false;

This pattern is particularly gruesome. Could we still keep masm.testUndefined()? Then we could use the original bailoutIf() form. I'm not even sure what bailing out to an unbound label does; I suspect we really want that to be an error case.

> masm.branchTestInt32(Assembler::NotEqual, tag, &notInt32);
> masm.j(cond, &notInt32);

I realize this isn't a patch review, but I couldn't help myself.
Comment 3 Nicolas B. Pierron [:nbp] 2011-09-28 12:36:52 PDT
(In reply to Nicolas B. Pierron [:pierron] from comment #1)
> (In reply to David Anderson [:dvander] from comment #0)
> > Created attachment 562949 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > wip v0
> > 
> > Marty noted that "masm.j" in platform-independent code was awkward in the
> > context of ARM, and now adding more paths like this, I agree. I would like
> > to shift to having a "test-and-branch" pattern like Nitro's branch32(),
> > branchPtr(), etc, as part of the MacroAssembler API. That includes existing
> > tests which take and return an Assembler::Condition.
> > 
> > Here's an example of testInt32 and testUndefined converted to the new scheme.
> 
> (cf 680315) What about the idea of packing the test inside the function and
> doing the jump outside the function.  Could that also be applied on ARM?  Or
> the test should be moved to the caller?  Otherwise could we have a
> call-and-branch with a test-and-return ?

in Bug 680315, I have the following pattern:

  wrapper:
    ...
    cmp 0x1fff4, %rax  ;   testMagic
    ret

  code:
    ...
    mov wrapper, %rax
    call %rax
    je bailout
    ...

Would it be possible on ARM ?
Comment 4 David Anderson [:dvander] 2011-09-28 16:37:30 PDT
> It would help my understanding to explain the problems that ARM faces with
> j().

Ah, it's not so much that ARM has a problem with it but that it has no "j*" instructions, they're "branch" or something instead. The way conditionals work is also very different in general.


> This pattern is particularly gruesome. Could we still keep
> masm.testUndefined()? 

Yeah, I agree. I'll keep the old pattern.

> > masm.branchTestInt32(Assembler::NotEqual, tag, &notInt32);
> > masm.j(cond, &notInt32);
> 
> I realize this isn't a patch review, but I couldn't help myself.

Thanks, good catch.
Comment 5 David Anderson [:dvander] 2011-09-28 16:37:53 PDT
Created attachment 563241 [details] [diff] [review]
patch
Comment 6 Sean Stangl [:sstangl] 2011-09-28 16:57:33 PDT
Comment on attachment 563241 [details] [diff] [review]
patch

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

::: js/src/ion/CodeGenerator.cpp
@@ +189,2 @@
>      cond = masm.testBooleanTruthy(false, value);
>      masm.j(cond, lir->ifFalse());

branchTestBooleanTruthy(), branchTestInt32Truthy(), ...?
Comment 7 David Anderson [:dvander] 2011-10-05 23:27:21 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/eafe88c524da

Note You need to log in before you can comment on or make changes to this bug.