Closed Bug 1064537 Opened 10 years ago Closed 10 years ago

Fold a ? a : 0 for number types

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files, 3 obsolete files)

> function (a) {
>    if (!a)
>        a = 0
> }

Is often used to test if the first argument exists and set a default value for it. Now this is annoying, since it doesn't test only for undefined, but also 0 takes the "false" path. Which is kinda sad, because we initialize it to 0.

So if this test is MIRType_Int32 / MIRType_Double we can just remove this branch.

This is part 1. There is also need for a MTest removal in DCE or UCE. I'll open a follow-up bug for that. But both patches combined improve raytrace with 10%.
Attached patch Fold (obsolete) — Splinter Review
Assignee: nobody → hv1989
Attachment #8485999 - Flags: review?(nicolas.b.pierron)
Attached patch Fold (obsolete) — Splinter Review
Also supports a?a:"" for strings
Attachment #8485999 - Attachment is obsolete: true
Attachment #8485999 - Flags: review?(nicolas.b.pierron)
Attachment #8486034 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8486034 [details] [diff] [review]
Fold

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

AFAIU, we are not even removing the MTest, do we have any plan on doing so, or the goal is just to make sure we can alias with the operand of the test?

::: js/src/jit/MIR.cpp
@@ +1139,5 @@
> +    }
> +
> +    MBasicBlock *pred = block()->getPredecessor(0)->getPredecessor(0);
> +    if (!pred->lastIns()->isTest())
> +        return this;

Do:

MBasicBlock *pred = block()->immediateDominator();
if (!pred || !pred->lastIns()->isTest())
    return this;

And remove the code above.

@@ +1143,5 @@
> +        return this;
> +
> +    // Fold a ? a : 0 and a ? a : "" for respectively number and string types.
> +    MTest *test = pred->lastIns()->toTest();
> +    bool firstIsTrueBranch = block()->getPredecessor(0) == test->ifTrue();

bool firstIsTrueBranch = test->ifTrue()->dominates(block()->getPredecessor(0));

@@ +1150,5 @@
> +
> +    if (test->input() != trueDef)
> +        return this;
> +    if (!falseDef->isConstant())
> +        return this;

The opposite might also be interesting for constant propagation.

   a ? "" : a

in which case the MPhi could be replaced by the corresponding MConstant.

@@ +1155,5 @@
> +
> +    // a ? a : 0 for number types
> +    if (IsNumberType(test->input()->type()) && falseDef->toConstant()->vp()->toNumber() == 0)
> +        return trueDef;
> +    // a ? a : "" for string types

This is not exact, what is handle by the current code is:

  a ? (…, a) : (…, "")

and respectively for numbers.
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 8486034 [details] [diff] [review]
> Fold
> 
> Review of attachment 8486034 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> AFAIU, we are not even removing the MTest, do we have any plan on doing so,
> or the goal is just to make sure we can alias with the operand of the test?

That is the follow-up bug 1064543. Which gives a 10% improvement on raytrace if implemented.
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> The opposite might also be interesting for constant propagation.
> 
>    a ? "" : a
> 
> in which case the MPhi could be replaced by the corresponding MConstant.

Oh yeah. I'll implement that too.

> @@ +1155,5 @@
> > +
> > +    // a ? a : 0 for number types
> > +    if (IsNumberType(test->input()->type()) && falseDef->toConstant()->vp()->toNumber() == 0)
> > +        return trueDef;
> > +    // a ? a : "" for string types
> 
> This is not exact, what is handle by the current code is:
> 
>   a ? (…, a) : (…, "")
> 
> and respectively for numbers.

I don't understand your comment here. This is a ? a : 0 which is implemented here?
(In reply to Hannes Verschore [:h4writer] from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > @@ +1155,5 @@
> > > +
> > > +    // a ? a : 0 for number types
> > > +    if (IsNumberType(test->input()->type()) && falseDef->toConstant()->vp()->toNumber() == 0)
> > > +        return trueDef;
> > > +    // a ? a : "" for string types
> > 
> > This is not exact, what is handle by the current code is:
> > 
> >   a ? (…, a) : (…, "")
> > 
> > and respectively for numbers.
> 
> I don't understand your comment here. This is a ? a : 0 which is implemented
> here?

What I meant is that you have no assertions that there is nothing before we return "a" nor "0".  You could have other function calls, variable declarations or anything else, and this code also match if statements.

So I just wanted to highlight that the comment is incomplete.
Attached patch Fold (obsolete) — Splinter Review
Yeah the immediateDominator is an easier way. I also added some more comments.
Attachment #8486034 - Attachment is obsolete: true
Attachment #8486034 - Flags: review?(nicolas.b.pierron)
Attachment #8487994 - Flags: review?(nicolas.b.pierron)
Attached patch FoldSplinter Review
This is better.
Attachment #8487994 - Attachment is obsolete: true
Attachment #8487994 - Flags: review?(nicolas.b.pierron)
Attachment #8487997 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8487997 [details] [diff] [review]
Fold

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

::: js/src/jit/LIR.h
@@ +506,5 @@
>          return type() == FLOAT32 || type() == DOUBLE || isSimdType();
>      }
>      uint32_t virtualRegister() const {
>          uint32_t index = (bits_ >> VREG_SHIFT) & VREG_MASK;
> +        //JS_ASSERT(index != 0);

This change shouldn't get pushed.
Comment on attachment 8487997 [details] [diff] [review]
Fold

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

::: js/src/jit/MIR.cpp
@@ +1148,5 @@
> +    MDefinition *falseDef = firstIsTrueBranch ? getOperand(1) : getOperand(0);
> +
> +    // Find
> +    // testDef ? testDef : constant or
> +    // testDef ? constant : testDef

comment nit:
  // Accept either
  //     testDef ? testDef : constant
  // or  testDef ? constant : testDef

@@ +1153,5 @@
> +    if (!trueDef->isConstant() && !falseDef->isConstant())
> +        return this;
> +
> +    MConstant *c = trueDef->isConstant() ? trueDef->toConstant() : falseDef->toConstant();
> +    MDefinition *testDef = (trueDef == c) ? falseDef : trueDef;

nit: s/testDef/testArg/

@@ +1198,5 @@
>  {
>      if (MDefinition *def = operandIfRedundant())
>          return def;
>  
> +    return foldsTernary();

instead of returning this, make it return nullptr, and use the same as operandIfRedundant test, as this allow chaining multiple strategies for folding the operation.
Attachment #8487997 - Flags: review?(nicolas.b.pierron) → review+
Fix the multi comments build warning, r=h4writer over irc:
https://hg.mozilla.org/integration/mozilla-inbound/rev/003a7359a668
https://hg.mozilla.org/mozilla-central/rev/6465e8acae5e
https://hg.mozilla.org/mozilla-central/rev/003a7359a668
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
What about negative zero?
(In reply to Simon Lindholm from comment #14)
> What about negative zero?

    // If testArg is a number type we can:
    // - fold testArg ? testArg : 0 to testArg
    // - fold testArg ? 0 : testArg to 0
    if (IsNumberType(testArg->type()) && c->vp()->toNumber() == 0)
        return trueDef;

The former is perfectly fine about this.  The latter is not.  I tried writing a testcase that fails but haven't succeeded.  I hit bug 1071799's assertion, but *after* correctly computing -0:

[jwalden@find-waldo-now src]$ dbg/js/src/js --no-threads --ion-eager -e 'var arr = new Float32Array([1, 2, 3, 4, 5, 6, 7, 8, 9, -0]); for (var i = 0; i < 10; i++) { var el = arr[i]; print(uneval(el ? +0 : el)); }'
0
0
0
0
0
0
0
0
0
-0
Assertion failure: opBlock->dominates(*block) (Instruction is not dominated by its operands), at /home/jwalden/moz/slots/js/src/jit/IonAnalysis.cpp:2043
Segmentation fault (core dumped)

It seems somehow the |testArg != test->input()| test fails, because

(gdb) p test->dump()
test64 = test loadtypedarrayelement53 block5 block6
$5 = void
(gdb) p trueDef->dump()
constant65 = constant 0x0
$6 = void
(gdb) p falseDef->dump()
typebarrier71 = typebarrier unbox70
$7 = void
(gdb) n
1219	    MDefinition *testArg = (trueDef == c) ? falseDef : trueDef;
(gdb) n
1220	    if (testArg != test->input())
(gdb) p testArg
$8 = (js::jit::MTypeBarrier *) 0x1e270f0
(gdb) n
1221	        return nullptr;

so somehow the test input is a loadtypedarrayelement, but the argument is a typedbarrier71.  No idea why that happens, I couldn't quickly massage it into something that made the two equal.

So, it does seem like there's a -0 issue at least sometimes here, but I don't know enough about the JIT to trigger it.
Attached patch Fix for -0Splinter Review
Testcase that fails. Fix is easy. Disallow MIRType_Double. I'll this testcase (with minor adjustments) when pushing.

function test(a) {
    return (a)?a:0;
}
function test2(a) {
    return (a)?0:a;
}

function isNegativeZero(x) {
    return x===0 && (1/x)===-Infinity;
}

test(0)
print(isNegativeZero(test(-0)))
print(isNegativeZero(test(-0)))
print(isNegativeZero(test2(-0)))
print(isNegativeZero(test2(-0)))
Attachment #8494035 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8494035 [details] [diff] [review]
Fix for -0

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

This might cause a merge conflict with your last patch, but this change sounds good.
Attachment #8494035 - Flags: review?(nicolas.b.pierron) → review+
Landed in bug 1072691 (by accident wrong bug number)
Depends on: 1090037
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: