A register that is allocated via GPRTemporary within one side or the other of an if () {} else {} is unsafe. The register allocation can cause a spill on the one path that doesn't happen on the other path. A subsequent use of the spilled register will load from the spilled location. This was the root of https://bugs.webkit.org/show_bug.cgi?id=111777 - "Crash when updating predictions below JSC::arrayProtoFuncForEach on tuaw.com article" I looked at the uses of GPRResult and GPRTemporary objects and found the following possibly similar issues: GPRTemporary structure(this) in SpeculativeJIT::compileObjectOrOtherLogicalNot for both DFGSpeculativeJIT32_64.cpp (~1598) and DFGSpeculativeJIT64.cpp (~1612) GPRTemporary structure(this) in SpeculativeJIT::compileObjectToObjectOrOtherEquality for both DFGSpeculativeJIT32_64.cpp (~1387) and DFGSpeculativeJIT64.cpp (~1411) GPRTemporary structure(this) in SpeculativeJIT::compilePeepHoleObjectToObjectOrOtherEquality for both DFGSpeculativeJIT32_64.cpp (~1488) and DFGSpeculativeJIT64.cpp (~1509) Tracked also in <rdar://problem/13406093>
Created attachment 193397 [details] Patch
Comment on attachment 193397 [details] Patch Attachment 193397 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17060952
Created attachment 193402 [details] Speculative patch for Qt build failure Added initialization to InvalidGPRReg for structureGPR in three places.
Comment on attachment 193402 [details] Speculative patch for Qt build failure Why was it wrong before?
(In reply to comment #4) > (From update of attachment 193402 [details]) > Why was it wrong before? Compiler error for uninitialized variable.
Comment on attachment 193402 [details] Speculative patch for Qt build failure View in context: https://bugs.webkit.org/attachment.cgi?id=193402&action=review r=me with one change suggested below: > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1609 > + GPRTemporary structure; > + GPRReg structureGPR = InvalidGPRReg; > + > + if (!m_jit.graph().globalObjectFor(m_currentNode->codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()) { > + GPRTemporary realStructure(this); // Only allocate on this path, before the branch > + structure.adopt(realStructure); > + structureGPR = structure.gpr(); > + } Let's use this model everywhere -- that way, we're always allocating our registers at the top of the function, and it's clearer that we got this right. Let's update this comment so it's clearer why we're doing this. Something like, "The masquerades as undefined case will use the structure register, so allocate it here. Do this at the top of the function to avoid branching around a register allocation."
> + if (!m_jit.graph().globalObjectFor(m_currentNode->codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()) { You mentioned that you were worried about this cost of this line of code. Let's put the result in a local variable and reuse it. That will resolve the cost, and read better too.
Committed r146100: <http://trac.webkit.org/changeset/146100>
Is it possible that this caused 20% memory regression on Mac? https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22Mac%20MountainLion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%2C%5B%22Mac%20Lion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%5D&zoom=%5B1363572327935.7915%2C1363623676769.9104%5D
(In reply to comment #9) > Is it possible that this caused 20% memory regression on Mac? I would put the probability of that below 1%.
(In reply to comment #10) > (In reply to comment #9) > > Is it possible that this caused 20% memory regression on Mac? > > I would put the probability of that below 1%. Okay, it's probably http://trac.webkit.org/changeset/146089 then.
(In reply to comment #9) > Is it possible that this caused 20% memory regression on Mac? > > https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22Mac%20MountainLion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%2C%5B%22Mac%20Lion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%5D&zoom=%5B1363572327935.7915%2C1363623676769.9104%5D Have you checked for leaks or does it appear to be just more usage? As Geoff says, not likely this change. I only moved around register allocations in the DFG JIT.