Closed Bug 689831 Opened 13 years ago Closed 13 years ago

IonMonkey: Change type testing API

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch wip v0 (obsolete) — Splinter Review
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.
(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 ?
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.
(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 ?
> 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.
Attached patch patchSplinter Review
Attachment #562949 - Attachment is obsolete: true
Attachment #563241 - Flags: review?(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(), ...?
Attachment #563241 - Flags: review?(sstangl) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/eafe88c524da
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.