Bug 148213 - Lets rename codeOriginIndex to callSiteIndex and get rid of CallFrame::Location.
Summary: Lets rename codeOriginIndex to callSiteIndex and get rid of CallFrame::Location.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-19 22:23 PDT by Saam Barati
Modified: 2015-10-08 21:53 PDT (History)
10 users (show)

See Also:


Attachments
patch (56.01 KB, patch)
2015-08-20 18:35 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (57.22 KB, patch)
2015-08-20 18:41 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (58.24 KB, patch)
2015-08-20 18:47 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (59.38 KB, patch)
2015-08-24 14:54 PDT, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-08-19 22:23:05 PDT
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.
Comment 1 Saam Barati 2015-08-20 18:16:35 PDT
I'm going to save HandlerInfo indexing for DFG exception handling patch.
Comment 2 Saam Barati 2015-08-20 18:35:44 PDT
Created attachment 259551 [details]
patch
Comment 3 Filip Pizlo 2015-08-20 18:37:17 PDT
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 4 Filip Pizlo 2015-08-20 18:39:57 PDT
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".
Comment 5 Saam Barati 2015-08-20 18:41:41 PDT
Created attachment 259552 [details]
patch

rebased.
Comment 6 WebKit Commit Bot 2015-08-20 18:43:51 PDT
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.
Comment 7 Saam Barati 2015-08-20 18:47:36 PDT
Created attachment 259554 [details]
patch

it should also build with ToT
Comment 8 WebKit Commit Bot 2015-08-20 18:51:05 PDT
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 9 Filip Pizlo 2015-08-21 13:33:23 PDT
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?
Comment 10 Saam Barati 2015-08-24 14:54:45 PDT
Created attachment 259782 [details]
patch
Comment 11 WebKit Commit Bot 2015-08-24 14:56:44 PDT
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.
Comment 12 Saam Barati 2015-08-24 15:06:09 PDT
Bug renamed locally in changelog,
Comment 13 Saam Barati 2015-08-24 16:40:06 PDT
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.
Comment 14 Saam Barati 2015-08-25 12:41:15 PDT
landed in:
http://trac.webkit.org/changeset/188932
Comment 15 Filip Pizlo 2015-10-08 20:03:08 PDT
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.
Comment 16 Saam Barati 2015-10-08 21:53:37 PDT
(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.