Closed Bug 677774 Opened 13 years ago Closed 13 years ago

IM: Implement conditional operators

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ascheff, Assigned: ascheff)

References

Details

Attachments

(4 files, 5 obsolete files)

I will implement conditional operators like <, >, <=, ==, you get the idea in IonMonkey
No type inference/specialization yet, this patch just adds classes and constructors mostly
Attachment #552537 - Flags: review?(dvander)
Comment on attachment 552537 [details] [diff] [review]
Adds MRelationalCompare. lt, gt, le, ge now supported in MIR

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

Should we call this MCompare instead? RelationalCompare is kind of long and we probably want it to include == and != later
Attachment #552537 - Flags: review?(dvander) → review+
I was going under the assumption that they would require fairly different TypePolicys.. I guess its a tradeoff between longer class names and some extra casing in type analysis.  I could shorten class names to MRelCompare and MEqCompare?
That seems okay. Another option would be passing in the TypePolicy to the constructor, and saving it in the node. Up to you.
After actually figuring out what the type policy for != and == would look like, I think they can go together in the same class
Adds MCompare and ComparePolicy. I *think* this is how it should look...
Attachment #552537 - Attachment is obsolete: true
Attachment #552821 - Flags: review?(dvander)
Comment on attachment 552821 [details] [diff] [review]
MCompare now with type analysis stuff

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

::: js/src/ion/MIR.cpp
@@ +462,5 @@
> +void
> +MCompare::infer(const TypeOracle::Binary &b)
> +{
> +    // Always a Boolean
> +    setResultType(MIRType_Boolean);

Set this in the constructor of the node, since the output doesn't change based on inference.

@@ +465,5 @@
> +    // Always a Boolean
> +    setResultType(MIRType_Boolean);
> +
> +    // If neither operand is an object, then we are idempotent
> +    if (!(b.lhs == MIRType_Object) && !(b.rhs == MIRType_Object))

simpler: (b.lhs != MIRTypeObject && b.rhs != MIRType_Object)

@@ +471,5 @@
> +
> +    // Set specialization
> +    if (b.lhs == MIRType_Int32 && b.rhs == MIRType_Int32) {
> +        specialization_ = MIRType_Int32;
> +    } else if (CoercesToDouble(b.lhs) && CoercesToDouble(b.rhs)) {

This check probably exists elsewhere too but it seems a little aggressive. For now we probably want ||, so (5.5 < 2) will specialize.

@@ +475,5 @@
> +    } else if (CoercesToDouble(b.lhs) && CoercesToDouble(b.rhs)) {
> +        specialization_ = MIRType_Double;
> +    } else {
> +        specialization_ = MIRType_None;
> +    }

No braces needed here, since all bodies are one-liners

::: js/src/ion/TypePolicy.cpp
@@ +152,5 @@
> +
> +    // If either input coerces to a double, we should respecialize to double
> +    if (CoercesToDouble(lhs->type()) || CoercesToDouble(rhs->type())) {
> +        specialization_ = MIRType_Double;
> +    }

No braces needed here

@@ +184,5 @@
> +
> +        // The type of the input must be an integer if it doesn't match
> +        // our specialization (which must be double).
> +        JS_ASSERT(in->type() == MIRType_Int32);
> +        MInstruction *replace = MToDouble::New(in);

This assert would trigger if you had (3.5 > 5). Here you want something similar to the Add policy:

  * If the input is a string or object, box it, then
  * If the input is not the specialization, insert ToInt32 or ToDouble
    depending on the specialization.
Attachment #552821 - Flags: review?(dvander)
Attached patch Comments addressed (obsolete) — Splinter Review
I made use of: blah < MIRType_String... I think this is much more meaningful than the CoercesToDouble function.  Perhaps CoercesToDouble(type) should return type < MIRType_String?
Attachment #552832 - Flags: review?(dvander)
Attachment #552821 - Attachment is obsolete: true
how about now?
Attachment #552832 - Attachment is obsolete: true
Attachment #552832 - Flags: review?(dvander)
Attachment #553307 - Flags: review?(dvander)
Comment on attachment 553307 [details] [diff] [review]
Made changes to type policy that we discussed

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

::: js/src/ion/TypePolicy.cpp
@@ +152,5 @@
> +
> +    // Check if any input would coerce to a double.
> +    if (CoercesToDouble(lhs->type()) || CoercesToDouble(rhs->type())) {
> +        specialization_ = MIRType_Double;
> +    }

No braces needed here.

@@ +186,5 @@
> +            replace = MUnbox::New(in, specialization_);
> +        else if (specialization_ == MIRType_Double)
> +            replace = MToDouble::New(in);
> +        else
> +            replace = MToInt32::New(in);

This won't work if the incoming type is an object, since MToInt32/Double() can't handle that. You want something like BinaryArithPolicy: if the type is object or string, box it. *then*, insert the conversion.

For example,
(obj < 5) should become: MCompare(MToInt32(MBox(obj)), MConstant(5))
(6.3 < 5) should become: MCompare(MConstant(6.3), MToDouble(MConstant(5)))

Sorry if this is confusing. The MToBlah opcodes weren't spec'd as of your patch since I forgot to land bug 677339, which I'll do now.

MToInt32: obj, string, undefined: not allowed
          null -> 0
          int32, bool -> int
          double -> int if fits in int
          value: deoptimize if obj, string, or undef

MToDouble: obj, string not allowed
           int32, double, bool -> double
           null -> 0
           undef -> NaN
           value: deoptimize if obj or string

The reason the box is necessary is just to make the IR cleaner. Otherwise, we would need an instruction which returns something but always deoptimizes, which is weird.
Attachment #553307 - Flags: review?(dvander)
Now, adjust inputs looks almost exactly like BinaryArithPolicy's
Attachment #553307 - Attachment is obsolete: true
Attachment #553590 - Flags: review?(dvander)
Comment on attachment 553590 [details] [diff] [review]
possibly last round of corrections

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

Looks good, r=me with some nits

::: js/src/ion/MIR.cpp
@@ +462,5 @@
> +void
> +MCompare::infer(const TypeOracle::Binary &b)
> +{
> +    // Always a Boolean
> +    setResultType(MIRType_Boolean);

Not needed since it's in the ctor

@@ +466,5 @@
> +    setResultType(MIRType_Boolean);
> +
> +    // If neither operand is an object, then we are idempotent
> +    if (b.lhs != MIRType_Object && b.rhs != MIRType_Object)
> +        setIdempotent();

Would it be simpler to move setIdempotent() to...

@@ +473,5 @@
> +    if (b.lhs < MIRType_String && b.rhs < MIRType_String) {
> +        if (CoercesToDouble(b.lhs) || CoercesToDouble(b.rhs))
> +            specialization_ = MIRType_Double;
> +        else
> +            specialization_ = MIRType_Int32;

Here? And then you don't need the extra check.

@@ +476,5 @@
> +        else
> +            specialization_ = MIRType_Int32;
> +    }
> +    else
> +        specialization_ = MIRType_None;

House style is to brace all branches if one branch is braced,

  if (...) {
    ...
  } else {
    one-liner
  }
Attachment #553590 - Flags: review?(dvander) → review+
Landed that patch, lowering comes next

http://hg.mozilla.org/projects/ionmonkey/rev/e029a3cf6a64
This patch adds a lowering pass and the discussed peephole optimization for the common case in which a compare is the input to a test instruction.
Attachment #553648 - Flags: review?(dvander)
Comment on attachment 553648 [details] [diff] [review]
Lowering pass for MCompare

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

::: js/src/ion/Lowering.cpp
@@ +144,5 @@
>      if (opd->type() == MIRType_Undefined || opd->type() == MIRType_Null)
>          return add(new LGoto(ifFalse));
>  
> +    // Check if the operand for this test is a compare operation. If it is, we need to emit
> +    // an LCompare*AndBranch rather than an LTest*AndBranch

I would say s/need/want/, and note that it's to fuse the compare and jump.

@@ +156,5 @@
> +                                              ifTrue, ifFalse));
> +        else if (comp->specialization() == MIRType_Double)
> +            JS_ASSERT("LCompareDAndBranch NYI");
> +        else
> +            JS_ASSERT("LCompareVAndBranch NYI");

Brace this if-chain since one block is a multi-liner. You also might want to remove the elseif/else and just let it fall through for now - better to have worse codegen than an assert, file a follow-up bug, and reference it in a :TODO: comment.

@@ +193,5 @@
> +        return define(new LCompareI(comp->compareOp(), useRegister(left), use(right)), comp);
> +    else if (comp->specialization() == MIRType_Double)
> +        JS_ASSERT("LCompareD NYI");
> +    else
> +        JS_ASSERT("LCompareV NYI");

Here, I would remove the else if/else and just put a JS_NOT_REACHED("NYI"). In the interest of not adding more NYIs, would it be hard to do double comparisons as well? :)

::: js/src/ion/MIR.h
@@ +846,5 @@
> +    MIRType specialization() const {
> +        return specialization_;
> +    }
> +
> +    JSOp compareOp() const {

I think this is "jsop()" elsewhere - if so, probably want to use that for consistency.
Attachment #553648 - Flags: review?(dvander) → review+
Lowering landed.  Code generation for CompareI and CompareIAndBranch is next

http://hg.mozilla.org/projects/ionmonkey/rev/0c863aeb0f75
Depends on: 679804
Here is a working implementation of code generation for LCompareI and LCompareIAndBranch with nothing fancy.  Looking for some feedback.  The todo list has optimizations with conditional set and predicting branches in loops.  Equality and doubles to come as well.
Attachment #554482 - Flags: review?(dvander)
Comment on attachment 554482 [details] [diff] [review]
First iteration of code generation for conditional operators

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

::: js/src/ion/LIR-Common.h
@@ +244,5 @@
>  };
>  
>  class LCompareI : public LInstructionHelper<1, 2, 0>
>  {
> +    AssemblerX86Shared::Condition cond_;

This is in LIR-Common so this should be Assembler. I think it's safe to assume every architecture's Assembler will provide flags like "Equal" and "GreaterThan". So everywhere in this file you reference AssemblerX86Shared, just use Assembler instead.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +143,5 @@
> +    const LAllocation *left = comp->getOperand(0);
> +    const LAllocation *right = comp->getOperand(1);
> +    LBlock *ifTrue = comp->ifTrue()->lir();
> +    LBlock *ifFalse = comp->ifFalse()->lir();
> +    AssemblerX86Shared::Condition cond = comp->condition();

You can just use "Assembler::"

@@ +152,5 @@
> +    // Take advantage of block fallthrough when possible
> +    if (isNextBlock(ifFalse)) {
> +        masm.j(cond, ifTrue->label());
> +    } else if (isNextBlock(ifTrue)) {
> +        masm.j(AssemblerX86Shared::inverseCondition(cond), ifFalse->label());

Same
Attachment #554482 - Flags: review?(dvander) → review+
A couple quick fixes, added test cases as well.

http://hg.mozilla.org/projects/ionmonkey/rev/7732904d4c8d
tiny optimization, use conditional set when possible.  Also this diff has the test cases which I forgot to include with the last patch
Attachment #555001 - Flags: review?(dvander)
Comment on attachment 555001 [details] [diff] [review]
Use conditional set to optimize compares with no branches

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

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +131,5 @@
> +    masm.cmpl(ToRegister(left), ToOperand(right));
> +
> +    // If the register we're defining is a single byte register,
> +    // take advantage of the setCC instruction
> +    if (ToRegister(def).code() & Registers::SingleByteRegs) {

This test won't work because code() does not return a mask. You want something like:

GeneralRegisterSet(Registers::SingleByteRegs).has(ToRegister(def))
Attachment #555001 - Flags: review?(dvander)
With fixes we discuessed
Attachment #555001 - Attachment is obsolete: true
Attachment #555234 - Flags: review?(dvander)
Attachment #555234 - Flags: review?(dvander) → review+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 721438
You need to log in before you can comment on or make changes to this bug.