This will help when we need more fine grained control over call sites that cause exceptions when implementing try/catch in the DFG. HandlerInfo should use a CallSiteIndex instead of an unsigned byte code index. CodeBlock's linear search through handlers should be keyed by CallSiteIndex. Also, we should get rid of CallFrame::Location. The first bit that encodes as CodeOriginIndex/BytecodeIndex can be determined by CodeBlock::jitType(). StructureStubInfo should also have a field that is a CallSiteIndex and Repatch should store this as the CallFrame's CallSiteIndex. It should no longer get this information from ExecState.
I'm going to save HandlerInfo indexing for DFG exception handling patch.
Created attachment 259551 [details] patch
Comment on attachment 259551 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=259551&action=review > Source/JavaScriptCore/interpreter/CallFrame.h:55 > + struct CallSiteIndex { > + explicit CallSiteIndex(uint32_t bits) > + : m_bits(bits) > + { } > +#if USE(JSVALUE32_64) > + explicit CallSiteIndex(Instruction* instruction) > + : m_bits(bitwise_cast<uint32_t>(instruction)) > + { } > +#endif > + inline uint32_t bits() const { return m_bits; } > + > + private: > + uint32_t m_bits; > + }; > + This should be in its own header. It should also be a class.
Comment on attachment 259551 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=259551&action=review > Source/JavaScriptCore/interpreter/CallFrameInlines.h:70 > +inline bool CallFrame::hasLocationAsCallSiteIndex() const > { > - ASSERT(hasLocationAsBytecodeOffset()); > ASSERT(codeBlock()); > - return Location::decode(locationAsRawBits()); > + switch (codeBlock()->jitType()) { > + case JITCode::DFGJIT: > + case JITCode::FTLJIT: > + return true; > + case JITCode::None: > + case JITCode::HostCallThunk: > + RELEASE_ASSERT_NOT_REACHED(); > + return false; > + default: > + return false; > + } > + > + RELEASE_ASSERT_NOT_REACHED(); > + return false; > } I don't get this. Does everyone have a location as a CallSiteIndex? Also, can we get rid of the term "location"? It appears that very location is encoded as a CallSiteIndex, so there shouldn't be a separate notion of "location".
Created attachment 259552 [details] patch rebased.
Attachment 259552 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:83: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:54: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:54: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:117: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:143: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:59: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 259554 [details] patch it should also build with ToT
Attachment 259554 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:83: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:54: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:54: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:117: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:143: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:59: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 259554 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=259554&action=review > Source/JavaScriptCore/interpreter/CallFrameInlines.h:34 > inline bool CallFrame::hasLocationAsBytecodeOffset() const Should this be "hasCallSiteIndexAsBytecodeOffset"? > Source/JavaScriptCore/interpreter/CallFrameInlines.h:53 > +inline bool CallFrame::hasLocationAsCallSiteIndex() const I still don't get this method name. Isn't the thing we store into the tag of the argument count always a CallSiteIndex?
Created attachment 259782 [details] patch
Attachment 259782 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:83: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:54: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:54: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:117: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:143: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:59: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Bug renamed locally in changelog,
Comment on attachment 259782 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=259782&action=review > Source/JavaScriptCore/interpreter/CallFrame.cpp:104 > + return codeOrigin.bytecodeIndex; This line of code is unreachable and removed locally.
landed in: http://trac.webkit.org/changeset/188932
Comment on attachment 259782 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=259782&action=review > Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:44 > - InlineCacheDescriptor(unsigned stackmapID, CodeOrigin codeOrigin, UniquedStringImpl* uid) > + InlineCacheDescriptor(unsigned stackmapID, CallSiteIndex callSite, UniquedStringImpl* uid) This change was wrong. An instance of an InlineCacheDescriptor corresponds to a single DFG IR inline cache. This will lower to a single LLVM IR patchpoint. But that patchpoint can get duplicated in LLVM IR (for example via jump threading or tail duplication). This is represented by having each InlineCacheDescriptor subclass have a Vector<> of inline caches. All of those inline caches will share the same CodeOrigin. But, they should not share the same CallSiteIndex. Each inline cache generated by one of these descriptors should have its own CallSiteIndex. That's because each inline cache will have its own patchpoint, and so its own StackMaps::Record, and so it will have its own unique set of registers (its own used register set, its own input registers, its own result register, and its own registers for OSR exit). Therefore each one needs to have its own exception handler, which means having its own CallSiteIndex.
(In reply to comment #15) > Comment on attachment 259782 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259782&action=review > > > Source/JavaScriptCore/ftl/FTLInlineCacheDescriptor.h:44 > > - InlineCacheDescriptor(unsigned stackmapID, CodeOrigin codeOrigin, UniquedStringImpl* uid) > > + InlineCacheDescriptor(unsigned stackmapID, CallSiteIndex callSite, UniquedStringImpl* uid) > > This change was wrong. An instance of an InlineCacheDescriptor corresponds > to a single DFG IR inline cache. This will lower to a single LLVM IR > patchpoint. But that patchpoint can get duplicated in LLVM IR (for example > via jump threading or tail duplication). This is represented by having each > InlineCacheDescriptor subclass have a Vector<> of inline caches. > > All of those inline caches will share the same CodeOrigin. But, they should > not share the same CallSiteIndex. Each inline cache generated by one of > these descriptors should have its own CallSiteIndex. That's because each > inline cache will have its own patchpoint, and so its own StackMaps::Record, > and so it will have its own unique set of registers (its own used register > set, its own input registers, its own result register, and its own registers > for OSR exit). Therefore each one needs to have its own exception handler, > which means having its own CallSiteIndex. Dang, ok. I'll fix it.