Bug 148213 – Lets rename codeOriginIndex to callSiteIndex and get rid of CallFrame::Location.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

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.

[Saam Barati]

I'm going to save HandlerInfo indexing for DFG exception handling patch.

[Saam Barati]

Created attachment 259551 [details]
patch

[Filip Pizlo]

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.

[Filip Pizlo]

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".

[Saam Barati]

Created attachment 259552 [details]
patch

rebased.

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 259554 [details]
patch

it should also build with ToT

[WebKit Commit Bot]

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.

[Filip Pizlo]

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?

[Saam Barati]

Created attachment 259782 [details]
patch

[WebKit Commit Bot]

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.

[Saam Barati]

Bug renamed locally in changelog,

[Saam Barati]

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.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/188932

[Filip Pizlo]

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.

[Saam Barati]

(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.