Created attachment 206266 [details] Patch

Comment on attachment 206266 [details] Patch Is it clear that this should be done under #if checks? This means that we will have a *ton* of code behind such checks. Seems like it might be better to do this as a giant patch.

(In reply to comment #2) > (From update of attachment 206266 [details]) > Is it clear that this should be done under #if checks? This means that we will have a *ton* of code behind such checks. Seems like it might be better to do this as a giant patch. Geoff and I talked about this some and agreed that we'd do a lot of smaller patches. I actually don't think there will be lots of #ifdef code. This code abstracts much of the placement of slots relative to the frame pointer. Therefore I suspect that other code will require both "grows up" and "grows down" code paths.

Comment on attachment 206266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206266&action=review Let's not #ifdef like this. I believe we can use math to avoid a mega-patch. I'll try to give some examples. This has the advantage of being able to test code incrementally / continuously. But if we can't use math, let's use a mega-patch. > Source/JavaScriptCore/interpreter/CallFrame.h:230 > + static int argumentOffset(int argument) { return s_firstArgumentOffset + argument; } Should be something like "return (JSStack::callFrameHeaderSize + 1 + argument) * s_argumentDirection;". > Source/JavaScriptCore/interpreter/CallFrame.h:231 > + static int argumentOffsetIncludingThis(int argument) { return s_thisArgumentOffset + argument; } Should be something like "return (JSStack::callFrameHeaderSize + argument) * s_argumentDirection;". > Source/JavaScriptCore/interpreter/JSStack.cpp:145 > void JSStack::gatherConservativeRoots(ConservativeRoots& conservativeRoots) > { > - conservativeRoots.add(begin(), getTopOfStack()); > +#if ENABLE(JSC_STACK_GROWS_DOWN) > + conservativeRoots.add(getTopOfStack(), highAddress()); > +#else > + conservativeRoots.add(lowAddress(), getTopOfStack()); > +#endif > } > > void JSStack::gatherConservativeRoots(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, DFGCodeBlocks& dfgCodeBlocks) > { > - conservativeRoots.add(begin(), getTopOfStack(), jitStubRoutines, dfgCodeBlocks); > +#if ENABLE(JSC_STACK_GROWS_DOWN) > + conservativeRoots.add(getTopOfStack(), highAddress(), jitStubRoutines, dfgCodeBlocks); > +#else > + conservativeRoots.add(lowAddress(), getTopOfStack(), jitStubRoutines, dfgCodeBlocks); > +#endif > } These should use a helper function that swaps begin and end if they're out of order. We already need this for the C stack (see MachineThreads::gatherFromCurrentThread), so perhaps it should be built into ConservativeRoots::add. > Source/JavaScriptCore/interpreter/JSStack.h:70 > +#if ENABLE(JSC_STACK_GROWS_DOWN) > + ArgumentCount = 6, > + CallerFrame = 5, > + Callee = 4, > + ScopeChain = 3, > + ReturnPC = 2, // This is either an Instruction* or a pointer into JIT generated code stored as an Instruction*. > + CodeBlock = 1, > +#else > ArgumentCount = -6, > CallerFrame = -5, > Callee = -4, > ScopeChain = -3, > ReturnPC = -2, // This is either an Instruction* or a pointer into JIT generated code stored as an Instruction*. > CodeBlock = -1, > +#endif These should multiply by a constant like argumentDirection or callFrameDirection.

(In reply to comment #4) > (From update of attachment 206266 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206266&action=review > > Let's not #ifdef like this. > > I believe we can use math to avoid a mega-patch. I'll try to give some examples. This has the advantage of being able to test code incrementally / continuously. > > But if we can't use math, let's use a mega-patch. > > > Source/JavaScriptCore/interpreter/CallFrame.h:230 > > + static int argumentOffset(int argument) { return s_firstArgumentOffset + argument; } > > Should be something like "return (JSStack::callFrameHeaderSize + 1 + argument) * s_argumentDirection;". > > > Source/JavaScriptCore/interpreter/CallFrame.h:231 > > + static int argumentOffsetIncludingThis(int argument) { return s_thisArgumentOffset + argument; } > > Should be something like "return (JSStack::callFrameHeaderSize + argument) * s_argumentDirection;". > > > Source/JavaScriptCore/interpreter/JSStack.cpp:145 > > void JSStack::gatherConservativeRoots(ConservativeRoots& conservativeRoots) > > { > > - conservativeRoots.add(begin(), getTopOfStack()); > > +#if ENABLE(JSC_STACK_GROWS_DOWN) > > + conservativeRoots.add(getTopOfStack(), highAddress()); > > +#else > > + conservativeRoots.add(lowAddress(), getTopOfStack()); > > +#endif > > } > > > > void JSStack::gatherConservativeRoots(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, DFGCodeBlocks& dfgCodeBlocks) > > { > > - conservativeRoots.add(begin(), getTopOfStack(), jitStubRoutines, dfgCodeBlocks); > > +#if ENABLE(JSC_STACK_GROWS_DOWN) > > + conservativeRoots.add(getTopOfStack(), highAddress(), jitStubRoutines, dfgCodeBlocks); > > +#else > > + conservativeRoots.add(lowAddress(), getTopOfStack(), jitStubRoutines, dfgCodeBlocks); > > +#endif > > } > > These should use a helper function that swaps begin and end if they're out of order. We already need this for the C stack (see MachineThreads::gatherFromCurrentThread), so perhaps it should be built into ConservativeRoots::add. > > > Source/JavaScriptCore/interpreter/JSStack.h:70 > > +#if ENABLE(JSC_STACK_GROWS_DOWN) > > + ArgumentCount = 6, > > + CallerFrame = 5, > > + Callee = 4, > > + ScopeChain = 3, > > + ReturnPC = 2, // This is either an Instruction* or a pointer into JIT generated code stored as an Instruction*. > > + CodeBlock = 1, > > +#else > > ArgumentCount = -6, > > CallerFrame = -5, > > Callee = -4, > > ScopeChain = -3, > > ReturnPC = -2, // This is either an Instruction* or a pointer into JIT generated code stored as an Instruction*. > > CodeBlock = -1, > > +#endif > > These should multiply by a constant like argumentDirection or callFrameDirection. I thought about using a sign constant. That eliminate much of the #ifdef's, but the stack committing and recommitting still needs separate paths. The reason I went down the #ifdef path is that I wanted the final code, that is after we change the direction to be at least as clean as the current code. I don't think we'll want to change the direction again. The difference in the two approaches is what the clean up patch looks like. With the #ifdef's, we know what code gets eliminated. The arithmetic approach requires either placing FIXME comments or remembering OOB. Been discussing this with Phil and he thinks that this is the least of our worries. The llint will have to add a negate instruction for register offsets when it reads them from the byte codes. Negation also has to happen for the JITs, but that is done in the generating code. I'll post a patch based on arithmetic for comparison.

> Been discussing this with Phil and he thinks that this is the least of our worries. The llint will have to add a negate instruction for register offsets when it reads them from the byte codes. Negation also has to happen for the JITs, but that is done in the generating code. Or we could change the encoded operands.

Created attachment 206359 [details] Updated patch Limited the #ifdef's to JSStack files. The ifdef's are needed due to doing arithmetic or comparisons with pointers or size_t type values. In both cases, we can't use the multiply by -1 or 1 trick. In some cases, we could create an #ifdef'ed help function to do <= comparisons or pointer arithmetic, but I felt that would obfuscate the reading of the code. If this patch isn't acceptable, then I think it is mega patch time.

Given what was discussed and the difficulty of making several small patches, we will work on one large patch to change the stack growth direction. That patch will be tracked in https://bugs.webkit.org/show_bug.cgi?id=118758 - "fourthTier: Change JSStack to grow from high to low addresses"

Comment on attachment 206359 [details] Updated patch Cleared review? from attachment 206359 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).