RESOLVED FIXED 39901
Simplified the host calling convention
https://bugs.webkit.org/show_bug.cgi?id=39901
Summary Simplified the host calling convention
Geoffrey Garen
Reported 2010-05-28 14:25:13 PDT
I'm posting three patches: - A behavior change patch that includes the interesting bits. - A global find-and-replace patch that makes everything compatible with the new calling convention. - A megapatch for the EWS bot to test, not for actual review.
Attachments
megapatch - DO NOT REVIEW (456.59 KB, patch)
2010-05-28 14:25 PDT, Geoffrey Garen
no flags
PART ONE: Functional code changes (98.31 KB, patch)
2010-05-28 14:26 PDT, Geoffrey Garen
barraclough: review+
PART TWO: Global search and replace (375.41 KB, patch)
2010-05-28 14:26 PDT, Geoffrey Garen
no flags
PART TWO: Global search and replace (375.41 KB, patch)
2010-05-28 14:33 PDT, Geoffrey Garen
sam: review+
Geoffrey Garen
Comment 1 2010-05-28 14:25:51 PDT
Created attachment 57378 [details] megapatch - DO NOT REVIEW
Geoffrey Garen
Comment 2 2010-05-28 14:26:20 PDT
Created attachment 57379 [details] PART ONE: Functional code changes
Geoffrey Garen
Comment 3 2010-05-28 14:26:52 PDT
Created attachment 57380 [details] PART TWO: Global search and replace
WebKit Review Bot
Comment 4 2010-05-28 14:30:32 PDT
Attachment 57380 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/bindings/js/JSClipboardCustom.cpp:70: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 111 files If any of these errors are false positives, please file a bug against check-webkit-style.
Geoffrey Garen
Comment 5 2010-05-28 14:33:54 PDT
Created attachment 57381 [details] PART TWO: Global search and replace Fixed style bot issue.
Eric Seidel (no email)
Comment 6 2010-05-28 14:53:20 PDT
Early Warning System Bot
Comment 7 2010-05-28 15:15:26 PDT
Sam Weinig
Comment 8 2010-05-28 15:30:05 PDT
Comment on attachment 57379 [details] PART ONE: Functional code changes JavaScriptCore/jit/JITOpcodes32_64.cpp:306 + subPtr(Imm32(16 - sizeof(void*)), stackPointerRegister); These stack alignment operations could use a comment. JavaScriptCore/jit/JITOpcodes.cpp:325 + }; These empty structs weird me out a bit.
Gavin Barraclough
Comment 9 2010-05-28 15:40:55 PDT
Comment on attachment 57379 [details] PART ONE: Functional code changes JavaScriptCore/interpreter/Interpreter.cpp:632 + m_registerFile.shrink(callFrame->registers() + callFrame->codeBlock()->m_numCalleeRegisters); Is this a functional change we should land in a separate patch, in case it has unexpected side-effects? JavaScriptCore/interpreter/Interpreter.cpp:710 + ASSERT(!callFrame->globalData().exception); Could be ASSERT(!callFrame->hadException()); JavaScriptCore/interpreter/Register.h:61 + Register& operator=(JSObject*); This is a world of danger. Please to be making this a static function (e.g. make people do something explicit like "Register::withFunction()"), or only write functions to the register file as JSValues. JavaScriptCore/jit/JITOpcodes32_64.cpp:281 + emitGetFromCallFrameHeaderPtr(RegisterFile::ScopeChain, regT1, regT0); I think you wanted to change both T1s to T0s here. JavaScriptCore/jit/JITOpcodes32_64.cpp:399 + emitGetFromCallFrameHeaderPtr(RegisterFile::ScopeChain, regT1, regT0); another T1 -> T0 required
Geoffrey Garen
Comment 10 2010-05-28 17:08:44 PDT
(In reply to comment #8) > (From update of attachment 57379 [details]) > JavaScriptCore/jit/JITOpcodes32_64.cpp:306 > + subPtr(Imm32(16 - sizeof(void*)), stackPointerRegister); > These stack alignment operations could use a comment. Fixed. > JavaScriptCore/jit/JITOpcodes.cpp:325 > + }; > These empty structs weird me out a bit. Got rid of them.
Geoffrey Garen
Comment 11 2010-05-28 17:11:18 PDT
(In reply to comment #9) > (From update of attachment 57379 [details]) > JavaScriptCore/interpreter/Interpreter.cpp:632 > + m_registerFile.shrink(callFrame->registers() + callFrame->codeBlock()->m_numCalleeRegisters); > Is this a functional change we should land in a separate patch, in case it has unexpected side-effects? Maybe. I chose to include it because it helped solve some of the stack overflow layout test changes. The alternative would be to change the expected results on even more tests, which I was trying to avoid. > JavaScriptCore/interpreter/Interpreter.cpp:710 > + ASSERT(!callFrame->globalData().exception); > Could be ASSERT(!callFrame->hadException()); Fixed. > > JavaScriptCore/interpreter/Register.h:61 > + Register& operator=(JSObject*); > This is a world of danger. Please to be making this a static function (e.g. make people do something explicit like "Register::withFunction()"), or only write functions to the register file as JSValues. Fixed. > > JavaScriptCore/jit/JITOpcodes32_64.cpp:281 > + emitGetFromCallFrameHeaderPtr(RegisterFile::ScopeChain, regT1, regT0); > I think you wanted to change both T1s to T0s here. > > JavaScriptCore/jit/JITOpcodes32_64.cpp:399 > + emitGetFromCallFrameHeaderPtr(RegisterFile::ScopeChain, regT1, regT0); > another T1 -> T0 required This API is a little weird, but it specifies that we're trying to fetch into T1, relative to T0. We want to preserve T0 for the sake of this following instruction: move(regT0, callFrameRegister); // Eagerly restore caller frame register to avoid loading from stack.
Gavin Barraclough
Comment 12 2010-05-28 17:34:55 PDT
Comment on attachment 57379 [details] PART ONE: Functional code changes olliej says yay too, so r+
Geoffrey Garen
Comment 13 2010-05-29 01:14:31 PDT
Everything's landed now, but I had to disabled ENABLE_JIT_OPTIMIZE_NATIVE_CALL on Windows for now, since it's mysteriously crashing on every test.
Oliver Hunt
Comment 14 2010-07-02 12:33:46 PDT
Comment on attachment 57378 [details] megapatch - DO NOT REVIEW Clearing the review flag as this isn't for review...
Note You need to log in before you can comment on or make changes to this bug.