676144 - IM: Implement codegen for LTestIAndBranch

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Scheff]

Attachment: Implementation of code generation for LTestIAndBranch on x64 only

Here is an implementation for x64 only.  For the TypePolicy, I'm sure this isn't the best way to do it... but for now, there is a separate TypePolicy for MTest called TestPolicy that does nothing.  During the lowering phase, MTest specializes to LTest*AndBranch according to the type of it's operand.

Comment 1

[Tom S [:evilpie]]

Just wondering, does the MacroAssembler optimize cmp(x, 0) to test(x, x) ?

Comment 2

[Andrew Scheff]

No. This might be noobish, but why is test(x, x) an optimization?

Comment 3

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

Comment on attachment 550261 [details] [diff] [review]
Implementation of code generation for LTestIAndBranch on x64 only

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

::: js/src/ion/LIR-Common.h
@@ +209,5 @@
> +    MBasicBlock *getIfTrue() const {
> +        return ifTrue;
> +    }
> +    MBasicBlock *getIfFalse() const {
> +        return ifFalse;

Could we append _ to these member names and then make the getters ifFalse, ifTrue?

::: js/src/ion/TypePolicy.h
@@ +114,5 @@
> +  public:
> +    bool respecialize(MInstruction *def);
> +    bool adjustInputs(MInstruction *def);
> +    bool useSpecializedInput(MInstruction *def, size_t index, MInstruction *special);
> +};

Do we still need a type policy? It's not totally clear yet but it looks like worst-case MTest can really take any input. In order of difficulty:

 * null, undefined: always false
 * object: always true
 * bool, integer: test != 0
 * string: test length != 0
 * double: test != 0, != NaN

And finally for boxed values these rules are pretty easy to inline.

::: js/src/ion/x64/CodeGenerator-x64.cpp
@@ +122,5 @@
> +    LBlock *ifFalse = test->getIfFalse()->lir();
> +
> +    masm.cmpq(ToOperand(opd), Imm32(0));
> +    masm.j(AssemblerX86Shared::Equal, ifFalse->begin()->toLabel()->label());
> +    masm.jmp(ifTrue->begin()->toLabel()->label());

I'm actually not sure why (or even, these days, if) "test" is faster than "cmp". One advantage would be that you don't need an immediate.

Here though, integers are 32-bit so you want to use cmpl (or testl), and this should also let you hoist into CodeGenerator-x86-shared. 

Also, we probably want to avoid emitting a jump to the next block if one of the cases falls through. Like:
 * If the false case is fall-through, negate the condition, elide
   the jmp.
 * If the true case is fall-through, elide the jmp.
 * If neither case is fall-through, emit both jumps.

Comment 4

[Andrew Scheff]

Attachment: Implementation of code generation for LTestIAndBranch on x86

This addresses all of your comments.  I also found a little mistake in the Assembler where our convention for lhs/rhs was not honored so that is fixed here as well.  Not sure if it's worth a separate bug.

Comment 5

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

Comment on attachment 550563 [details] [diff] [review]
Implementation of code generation for LTestIAndBranch on x86

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

Looks good, r=me with one nit:

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +139,5 @@
> +    } else if (isNextBlock(ifTrue)) {
> +        masm.j(AssemblerX86Shared::Zero, ifFalse->begin()->toLabel()->label());
> +    } else {
> +        masm.j(AssemblerX86Shared::Zero, ifFalse->begin()->toLabel()->label());
> +        masm.jmp(ifTrue->begin()->toLabel()->label());

How about adding a label() accessor to LBlock?

Comment 6

[Andrew Scheff]

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