IM: Implement codegen for LTestIAndBranch

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Andrew Scheff, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 550261 [details] [diff] [review]
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.
Attachment #550261 - Flags: review?(dvander)
Just wondering, does the MacroAssembler optimize cmp(x, 0) to test(x, x) ?
(Reporter)

Comment 2

6 years ago
No. This might be noobish, but why is test(x, x) an optimization?
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.
Attachment #550261 - Flags: review?(dvander)
(Reporter)

Comment 4

6 years ago
Created attachment 550563 [details] [diff] [review]
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.
Attachment #550261 - Attachment is obsolete: true
Attachment #550563 - Flags: review?(dvander)
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?
Attachment #550563 - Flags: review?(dvander) → review+
(Reporter)

Comment 6

6 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/c416300d0b62
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.