1064537 - Fold a ? a : 0 for number types

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

> 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%.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Fold

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Fold

Also supports a?a:"" for strings

Comment 3

[Nicolas B. Pierron [:nbp]]

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.

Comment 4

[Hannes Verschore [:h4writer]]

(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.

Comment 5

[Hannes Verschore [:h4writer]]

(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?

Comment 6

[Nicolas B. Pierron [:nbp]]

(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.

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Fold

Yeah the immediateDominator is an easier way. I also added some more comments.

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Fold

This is better.

Comment 9

[Hannes Verschore [:h4writer]]

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 10

[Nicolas B. Pierron [:nbp]]

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.

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6465e8acae5e

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Fix the multi comments build warning, r=h4writer over irc:
https://hg.mozilla.org/integration/mozilla-inbound/rev/003a7359a668

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6465e8acae5e
https://hg.mozilla.org/mozilla-central/rev/003a7359a668

Comment 14

[Simon Lindholm]

What about negative zero?

Comment 15

[Jeff Walden [:Waldo]]

(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.

Comment 16

[Hannes Verschore [:h4writer]]

Attachment: Fix for -0

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)))

Comment 17

[Nicolas B. Pierron [:nbp]]

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.

Comment 18

[Hannes Verschore [:h4writer]]

Landed in bug 1072691 (by accident wrong bug number)