For whatever reason, this initially only appeared on my machine in truthies.js, but I've reduced it, and it now reproduces on other people's repositories.

MTest was missing a type policy, so MTest(string) was lowered to LTestIAndBranch.

Comment on attachment 597006 [details] [diff] [review] Patch Review of attachment 597006 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks fine (I probably shouldn't review too much of the lowering phase, since I've never seen the code before, but I am concerned about the code that is generated by the builder.

I suspect that the code that is generated by the builder is wrong, but I don't know enough about JS to get it to show anything obvious. The code that is currently generated for if (a || b) { return 1; } return 0; is (In pseudo assembly) block 1: a_ <- toBool(a); if (a_) goto: 3 goto: 2 block 2: tmp <- b goto: 4 block 3: tmp <- a goto: 4 block 4: tmp_ <- toBool(tmp) if (tmp_) goto ret_1 goto ret_0 The fact that when the test should short circuit we are testing the first element twice seems quite wrong to me, but it currently seems to be nothing more than an efficency issue.

(In reply to Marty Rosenberg [:Marty] from comment #3) > > The fact that when the test should short circuit we are testing the first > element twice seems quite wrong to me, but it currently seems to be nothing > more than an efficency issue. It's based on the bytecode we emit (the interpreter and JM do the same). It's okay to test the LHS twice since ValueToBoolean has no observable side-effects. IonBuilder could optimize it by keeping track of branches or something but I think we should wait with that until we have better fuzz/test coverage.

Asking for another review per comment 2.

Comment on attachment 597823 [details] [diff] [review] Patch Review of attachment 597823 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/TypePolicy.cpp @@ +172,5 @@ > + case MIRType_String: > + { > + MStringLength *length = MStringLength::New(op); > + ins->block()->insertBefore(ins, length); > + ins->replaceOperand(0, length); At first I wasn't sure whether this was the right place, but I think it is, since the goal is to convert from string to boolean.