689831 - IonMonkey: Change type testing API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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]]

(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]]

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]]

(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] - inactive, e-mail if emergency]

> 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] - inactive, e-mail if emergency]

Attachment: patch

Comment 6

[Sean Stangl [:sstangl]]

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] - inactive, e-mail if emergency]

http://hg.mozilla.org/projects/ionmonkey/rev/eafe88c524da