Can we output smaller code in MTest by using TI info for the object-or-null case?

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla32
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Right now we only look at opd->type().  However this is pretty coarse-grained.  In particular, in the I assume common object-or-null case it will return MIRType_Value and then we'll go the full LTestVAndBranch route.

It might be good to look at resultTypeSet to see if it's basically "object types or null" and if so just check for null.  Maybe the same thing for "object types or undefined".

We'd only want that if !operandMightEmulateUndefined(), which means we should fix bug 1004169 first.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
Blocks: 1004169
No longer depends on: 1004169
Attachment #8415772 - Attachment is obsolete: true
Attachment #8415772 - Flags: review?(jwalden+bmo)
Attachment #8416092 - Flags: review?(jwalden+bmo) → review?(jdemooij)
Comment on attachment 8416092 [details] [diff] [review]
Improve codegen in testValueTruthyKernel to emit as few tests as we can get away with given our type inference information

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

Nice work, good to have this fixed. r=me with nits below addressed.

In general it seems like we handle the monomorphic cases pretty well these days, but especially when we end up with "Value" there's still room for improvement.

::: js/src/jit/CodeGenerator.cpp
@@ +525,5 @@
> +    bool mightBeDouble = valueMIR->mightBeType(MIRType_Double);
> +    int tagCount = int(mightBeUndefined) + int(mightBeNull) +
> +        int(mightBeBoolean) + int(mightBeInt32) + int(mightBeObject) +
> +        int(mightBeString) + int(mightBeDouble);
> +

Please add an assert here, like this:

MOZ_ASSERT_IF(!valueMIR->emptyResultTypeSet(), tagCount > 0);

So that we get a nice assertion failure if we add a new MIRType and forget to handle it here.

@@ +555,5 @@
> +            masm.branchTestBoolean(Assembler::NotEqual, tag, &notBoolean);
> +        masm.branchTestBooleanTruthy(false, value, ifFalsy);
> +        masm.jump(ifTruthy);
> +        if (tagCount != 1)
> +            masm.bind(&notBoolean);

You could remove the condition here (and similar cases below); it's ok to bind a label without jumping to it. If you think it's clearer this way that's also fine, your call.

@@ +587,5 @@
> +                masm.bind(&notObject);
> +        } else {
> +            if (tagCount != 1)
> +                masm.branchTestObject(Assembler::Equal, tag, ifTruthy);
> +            // else just fall through to truthiness

Nit: full sentence:

// Else just fall through to truthiness.

@@ +594,2 @@
>      } else {
> +        MOZ_ASSERT(!ool, "We better not have a stub we don't plan to initialize");

Nit: we're not initializing it, just jumping to it so "We better not have an unused OOL path"

@@ +602,5 @@
> +        if (tagCount != 1)
> +            masm.branchTestString(Assembler::NotEqual, tag, &notString);
> +        masm.branchTestStringTruthy(false, value, ifFalsy);
> +        masm.jump(ifTruthy);
> +        if (tagCount != 1)

You could move the masm.jump(ifTruthy) inside this "if", with a comment after the if like you added above for the object case.

@@ +680,5 @@
> +    // practice, we don't need the OutOfLineTestObject if the input to our test
> +    // is not an object.
> +    MDefinition* input = lir->mir()->input();
> +    if (lir->mir()->operandMightEmulateUndefined() &&
> +        input->mightBeType(MIRType_Object)) {

Nit: this fits on one line.
Attachment #8416092 - Flags: review?(jdemooij) → review+
> MOZ_ASSERT_IF(!valueMIR->emptyResultTypeSet(), tagCount > 0);

Perfect, thanks.  I was looking for a way to make sure we didn't miss new types getting added, and couldn't think of one.

> Nit: we're not initializing it, just jumping to it so "We better not have an unused OOL
> path"

Actually, we are in fact initializing it.  testObjectEmulatesUndefined calls testObjectEmulatesUndefinedKernel, which calls setInputAndTargets on the OutOfLineTestObject.  And if that call doesn't happen, then when OutOfLineTestObject::accept is called it'll assert that setInputAndTargets got called, then in an opt build pass its null pointer members to emitOOLTestObject which will crash with a null deref.

I'll flesh out the assertion message a bit to make that clearer.  How about:

        MOZ_ASSERT(!ool,
                   "We better not have an unused OOL path, since the code generator will try to "
                   "generate code for it but we never set up its labels, which will cause null "
                   "derefs of those labels.");


> You could move the masm.jump(ifTruthy) inside this "if"

Good catch.  Did that in the boolean and int32 cases too.
https://hg.mozilla.org/mozilla-central/rev/4c874962ee02
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1005590
You need to log in before you can comment on or make changes to this bug.