1047266 - Folding of MTest

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Not tested yet.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Improved folding

This patch passes jit-tests.

I wanted to replace MTest with MGoto, but the UCE in GVN complaints on this. Is this problem known?

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Improved folding

Forgot to qref

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Part 2

This fails jit-tests due to uce in gvn. Is this a known issue or should I look further to fix it?

Comment 5

[Dan Gohman [:sunfish]]

Bug 1029830 also contains a patch which does this, along with changes to UCE. However, there is a bug which is triggered by folding MTest into MGoto, which is described in that bug.

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 8466268 [details] [diff] [review]
Improved folding

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

::: js/src/jit/Lowering.cpp
@@ +734,5 @@
> +            return add(new(alloc()) LGoto(result ? ifTrue : ifFalse));
> +          }
> +          default:
> +            break;
> +        }

This looks good, and I believe you can simplify it even further; the opd->type() check should be unnecessary because valueToBoolean() should work on any type that's valid in this context.

::: js/src/jit/MIR.cpp
@@ +286,5 @@
> +        return MTest::New(alloc, MConstant::New(alloc, BooleanValue(false)), ifTrue(), ifFalse());
> +
> +    // A symbol is always true.
> +    if (op->type() == MIRType_Symbol)
> +        return MTest::New(alloc, MConstant::New(alloc, BooleanValue(true)), ifTrue(), ifFalse());

Instead of creating an MTest with a constant condition, it'd be simpler to just create an MGoto directly.

Folding MTest with a symbol operand is something the patches in bug 1029830 don't have yet.

@@ +2818,5 @@
>          return MConstant::New(alloc, BooleanValue(true));
>  
> +    // NOT of a symbol is always false.
> +    if (input()->type() == MIRType_Symbol)
> +        return MConstant::New(alloc, BooleanValue(false));

This doesn't fold away any successors, so it'd actually be nice to split this part out into a separate patch, as it should be safe from the bug.

::: js/src/jit/ValueNumbering.cpp
@@ +537,5 @@
> +    for (uint32_t i = 0; i < newControl->numOperands(); i++) {
> +        MDefinition *def = newControl->getOperand(i);
> +        if (def->isConstant() && def->block() == nullptr)
> +            control->block()->insertBefore(control->toInstruction(), def->toInstruction());
> +    }

I'd be nice to avoid having to do this. If the code in the current patch can be changed to generate MGoto directly instead of MTest with a constant condition, then we don't need this.

Comment 7

[Dan Gohman [:sunfish]]

Comment on attachment 8466271 [details] [diff] [review]
Part 2

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

I think it would simply things to fold these two patches together into one patch.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8466268 [details] [diff] [review]
Improved folding

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

I now know that you intended this patch specifically to avoid the bug in bug 1029830, so overall this patch looks good. You can disregard several of my earlier comments here (except one, as noted below). The main question is about the code which adds instructions to blocks after foldsTo.

::: js/src/jit/Lowering.cpp
@@ +734,5 @@
> +            return add(new(alloc()) LGoto(result ? ifTrue : ifFalse));
> +          }
> +          default:
> +            break;
> +        }

This is still pertinent.

::: js/src/jit/MIR.cpp
@@ +286,5 @@
> +        return MTest::New(alloc, MConstant::New(alloc, BooleanValue(false)), ifTrue(), ifFalse());
> +
> +    // A symbol is always true.
> +    if (op->type() == MIRType_Symbol)
> +        return MTest::New(alloc, MConstant::New(alloc, BooleanValue(true)), ifTrue(), ifFalse());

Ok, disregard my earlier comment here.

@@ +303,5 @@
> +          }
> +          default:
> +            break;
> +        }
> +    }

As above in LIRGenerator::visitTest, it shouldn't be necessary to check op->type() here; toConstant()->valueToBoolean() should be enough.

@@ +2818,5 @@
>          return MConstant::New(alloc, BooleanValue(true));
>  
> +    // NOT of a symbol is always false.
> +    if (input()->type() == MIRType_Symbol)
> +        return MConstant::New(alloc, BooleanValue(false));

Disregard my earlier comment here as well.

::: js/src/jit/ValueNumbering.cpp
@@ +537,5 @@
> +    for (uint32_t i = 0; i < newControl->numOperands(); i++) {
> +        MDefinition *def = newControl->getOperand(i);
> +        if (def->isConstant() && def->block() == nullptr)
> +            control->block()->insertBefore(control->toInstruction(), def->toInstruction());
> +    }

And, ignore my earlier comment here too.

Although, this is a little ugly still. What would you think about requiring the foldsTo methods to add any extra instructions beyond the final one to the block themselves?

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8466268 [details] [diff] [review]
Improved folding

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

::: js/src/jit/Lowering.cpp
@@ +734,5 @@
> +            return add(new(alloc()) LGoto(result ? ifTrue : ifFalse));
> +          }
> +          default:
> +            break;
> +        }

What about MIRType_Objects? While valueToBoolean will indeed give a result. But we are offthread. ToBooleanSlow calls EmulatesUndefined which does a UncheckedUnwrap. This unwrapping could race with the MainThread, since we aren't locking anywhere. So I added everything I know was definitely save. I think we can do JSString too, since the length is always in the same place.

::: js/src/jit/MIR.cpp
@@ +303,5 @@
> +          }
> +          default:
> +            break;
> +        }
> +    }

Not true. e.g. MIRType_Boolean and MIRType_Object are not in this list. It doesn't make sense for MIRType_Boolean. For MIRType_Object I have the same objection. MIRType_String should normally be possible.

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8466268 [details] [diff] [review]
Improved folding

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

::: js/src/jit/Lowering.cpp
@@ +734,5 @@
> +            return add(new(alloc()) LGoto(result ? ifTrue : ifFalse));
> +          }
> +          default:
> +            break;
> +        }

If valueToBoolean is not thread-safe to call on some types, it's very non-obvious, so it should be commented and probably also asserted in no uncertain terms in MConstant::valueToBoolean.

Also, we currently call valueToBoolean from several other places in the JIT without checking the type(), so if this is unsafe, we should fix those places too.

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Improved folding

Should address all your comments.

Comment 12

[Dan Gohman [:sunfish]]

Comment on attachment 8474629 [details] [diff] [review]
Improved folding

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

::: js/src/jit/MIR.cpp
@@ +299,5 @@
> +    }
> +
> +    if (constant) {
> +        block()->insertBefore(this, constant);
> +        return MTest::New(alloc, constant, ifTrue(), ifFalse());

This doesn't add the constant to the block. The previous patch had code to add in in GVN, and this patch doesn't, so foldsTo functions which create more than one instruction have to add those instruction to blocks themselves (this is as I prefer it, as having GVN hunt around the expression tree for things which need to be added is tricky).

However, I have finally found a fix for the mochitest failure that had been holding up bug 1029830. The patches for that bug are now posted and awaiting review. With those patches, we can just emit an MGoto straightaway here.

Comment 13

[Dan Gohman [:sunfish]]

I'm marking bug 1029830 as a dependency here. The patch here has some things that those patches don't such as the folding of Symbols in boolean contexts, so we should take another look once those patches land.

Comment 14

[Dan Gohman [:sunfish]]

Comment on attachment 8474629 [details] [diff] [review]
Improved folding

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

::: js/src/jit/MIR.cpp
@@ +299,5 @@
> +    }
> +
> +    if (constant) {
> +        block()->insertBefore(this, constant);
> +        return MTest::New(alloc, constant, ifTrue(), ifFalse());

Ok, bug 1029830 has landed now, which enabled some of these optimizations, but not all. Would you mind rebasing this patch, and retaining the union of the optimizations here :-)?

Comment 15

[Hannes Verschore [:h4writer]]

Attachment: Some fold improvements

Rebased.

Comment 16

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd510ef77a0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cd510ef77a0