Consider this function (reduced from string-fasta): -- function fastaRandom(n) { var line = Array(60); while (n>0) { for (var i=0; i<line.length; i++) { } n = n - 1; } } -- Currently we insert an unnecessary LValueToInt32 instruction before the LSubI for "n - 1". The problem is that we enter the inner loop via OSR, and guess the types of the stack slots like this: -- for (uint32 i = 0; i < osrBlock->stackDepth(); i++) slotTypes[i] = predecessor->getSlot(i)->type(); oracle->getNewTypesAtJoinPoint(script, loopHead, slotTypes); -- This fails if predecessor->getSlot(i) is a phi (its type is always MIRType_Value before phis are specialized). This problem is more common now that phis are placed eagerly. There are two ways to fix this: 1) Track slot types in the compiler, like JM. On every SETLOCAL, SETARG etc we have to update the slot type. 2) Special-case MOSRValue operands during phi specialization. E.g. if we have Phi(ToInt32, OSRValue) we specialize the Phi as Int32 and insert an unbox for the OSRValue. I'm leaning towards (2) because it seems a bit simpler. Any thoughts?

#2 is not necessarily correct. If we have an integer variable that may become a double in the loop, then the MPhi could plausibly be MPhi(MIRType_Int32, MIRType_Double), but since the MIRType_Double is masked by an MOsrValue, we would specialize to MIRType_Int32 and bail. The current scheme works in that case due to the call to oracle->getNewTypesAtJoinPoint(). Is there some way to get the type information we need from TI?

AFAIK it's not possible to get this information directly from TI. The only way is to track types in the compiler, like JM, but that seems also complicated. This patch unboxes OsrValues during type analysis, but only if we have to (the previous instruction is a phi and there's no "new type" at the join point).

Comment on attachment 611467 [details] [diff] [review] Patch Review of attachment 611467 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonAnalysis.cpp @@ +387,5 @@ > + continue; > + > + JS_ASSERT(in->type() == MIRType_Value); > + > + if (phi->type() <= MIRType_Null) { Don't use <=; explicitly test against Null and Undefined. ::: js/src/ion/LIR-Common.h @@ +2458,5 @@ > return getTemp(0)->output(); > } > }; > > +class LGuardType : public LInstructionHelper<0, BOX_PIECES, 0> Other barrier/guard instructions define an SSA value -- they use that to make sure that even though the guard isMoveable(), it still must execute before any instruction that consumes the guard's input. I think that is necessary even here to be strictly correct (i.e., even though nothing currently moves instructions around in the invalid way, we should make the invalid way impossible by expressing dependencies). ::: js/src/ion/MIR.h @@ +2275,5 @@ > class MOsrValue : public MUnaryInstruction > { > private: > ptrdiff_t frameOffset_; > + bool usePredecessorType_; This is worthy of an explanatory comment. I really don't like this, but I don't have any better ideas at the moment. @@ +3485,5 @@ > return AliasSet::Load(AliasSet::ObjectFields); > } > }; > > +class MGuardType An explanatory comment referencing MUnbox may be useful here, lest people get the idea that MGuardType is the common mechanism for type checking. Alternatively, is there really no way to use MTypeBarrier? Could we create a new TypeSet that only contains Null or Undefined, and reuse that machinery?

(In reply to Sean Stangl from comment #3) > > Don't use <=; explicitly test against Null and Undefined. Yeah I wasn't sure about it but we use the same condition in a similar if-statement, I'll change them both. > > Alternatively, is there really no way to use MTypeBarrier? Could we create a > new TypeSet that only contains Null or Undefined, and reuse that machinery? I think it's best to use MTypeBarrier only for the TI barriers; for instance, when it bails out it monitors the value on top of the stack, but we don't want/need to do that here. I just realized a problem with this patch though: like phis, MTypeBarrier (and probably more instructions) also returns a Value so it has similar problems. We could unbox them eagerly but I'll try #1 today and see if that's really more complicated.

Instead of guessing the type, get precise type info from the oracle. This means we can do an infallible unbox instead of fallible. To get the type information we have to walk the bytecode until we reach the OSR PC, but this is very fast. Both JM and my chunked compilation patch do something similar and I've never seen these functions in a profile.

Comment on attachment 625660 [details] [diff] [review] Patch v2 Review of attachment 625660 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. With the recently-landed patch limiting scripts based on length this seems OK. ::: js/src/ion/IonBuilder.cpp @@ +1803,5 @@ > jsbytecode *ifne = pc + ifneOffset; > JS_ASSERT(ifne > pc); > > // Verify that the IFNE goes back to a loophead op. > + DebugOnly<jsbytecode *> loopHead = GetNextPc(pc); This will still call GetNextPc(pc) in opt builds, right? We could just not use a loopHead variable, and instead s/loopHead/GetNextPc(pc)/ in the JS_ASSERTs below. @@ +1882,5 @@ > loopEntry = GetNextPc(bodyStart); > } > jsbytecode *loopHead = bodyStart; > JS_ASSERT(JSOp(*bodyStart) == JSOP_LOOPHEAD); > JS_ASSERT(ifne + GetJumpOffset(ifne) == bodyStart); Can get rid of loopHead: unused. ::: js/src/ion/TypeOracle.cpp @@ +174,3 @@ > ScriptAnalysis *analysis = script->analysis(); > > + while (pc < osrPc) { Deserves a comment.

https://hg.mozilla.org/projects/ionmonkey/rev/efd4c7fc0697 (In reply to Sean Stangl from comment #6) > > This will still call GetNextPc(pc) in opt builds, right? We could just not > use a loopHead variable, and instead s/loopHead/GetNextPc(pc)/ in the > JS_ASSERTs below. Good catch, done. > > Can get rid of loopHead: unused. loopHead was used near the end of the method so I had to keep it.