Last Comment Bug 676144 - IM: Implement codegen for LTestIAndBranch
: IM: Implement codegen for LTestIAndBranch
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Mac OS X
: -- normal (vote)
: ---
Assigned To: general
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-08-02 17:28 PDT by Andrew Scheff
Modified: 2011-08-04 15:24 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Implementation of code generation for LTestIAndBranch on x64 only (6.13 KB, patch)
2011-08-02 17:28 PDT, Andrew Scheff
no flags Details | Diff | Splinter Review
Implementation of code generation for LTestIAndBranch on x86 (8.65 KB, patch)
2011-08-03 17:23 PDT, Andrew Scheff
dvander: review+
Details | Diff | Splinter Review

Description Andrew Scheff 2011-08-02 17:28:39 PDT
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.
Comment 1 Tom Schuster [:evilpie] 2011-08-02 17:39:56 PDT
Just wondering, does the MacroAssembler optimize cmp(x, 0) to test(x, x) ?
Comment 2 Andrew Scheff 2011-08-02 17:52:22 PDT
No. This might be noobish, but why is test(x, x) an optimization?
Comment 3 David Anderson [:dvander] 2011-08-02 23:31:19 PDT
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());
> +>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 2011-08-03 17:23:02 PDT
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.
Comment 5 David Anderson [:dvander] 2011-08-03 17:35:50 PDT
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());
> +>begin()->toLabel()->label());

How about adding a label() accessor to LBlock?

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