Bug 149970 - FTL should generate a unique OSR exit for each duplicated OSR exit stackmap intrinsic.
Summary: FTL should generate a unique OSR exit for each duplicated OSR exit stackmap i...
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: 149409
  Show dependency treegraph
 
Reported: 2015-10-09 15:19 PDT by Saam Barati
Modified: 2015-10-19 13:32 PDT (History)
11 users (show)

See Also:


Attachments
patch (37.83 KB, patch)
2015-10-12 19:37 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-10-09 15:19:44 PDT
Currently, when we compile an OSR exit, we just find a single Stackmap::Record that matches
the stackmapID. But there may be many such records. We need the actual one that corresponds
to the proper OSR exit.
Comment 1 Saam Barati 2015-10-12 19:37:03 PDT
Created attachment 262967 [details]
patch
Comment 2 WebKit Commit Bot 2015-10-12 19:40:01 PDT
Attachment 262967 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:48:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:49:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:50:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:54:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:393:  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: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2015-10-16 12:12:08 PDT
Oops ... I closed the wrong bug.  Re-opening.
Comment 4 Mark Lam 2015-10-19 11:05:01 PDT
Comment on attachment 262967 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262967&action=review

> Source/JavaScriptCore/ChangeLog:12
> +        a signle OSR exit data structure. Then, when we compiled an OSR exit, we 

"single" => "single".
Comment 5 Filip Pizlo 2015-10-19 11:07:47 PDT
Comment on attachment 262967 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262967&action=review

> Source/JavaScriptCore/ftl/FTLJITCode.h:89
> +    SegmentedVector<OSRExitDescriptor, 8> osrExitDescriptors;

This should probably go to FTL::State since it's not used after FTL::compile().

> Source/JavaScriptCore/ftl/FTLStackMaps.h:129
> -    typedef HashMap<uint32_t, Vector<Record>, WTF::IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> RecordMap;
> +    struct RecordMapValue {
> +        Vector<Record> records;
> +        Vector<uint32_t> indices;
> +    };
> +    typedef HashMap<uint32_t, RecordMapValue, WTF::IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> RecordMap;

What about using Vector<RecordAndIndex> instead of RecordMapValue?  It's usually better to have one vector that contains a tuple, than a tuple containing multiple vectors.
Comment 6 Saam Barati 2015-10-19 13:30:43 PDT
landed in:
http://trac.webkit.org/changeset/191313
Comment 7 Saam Barati 2015-10-19 13:32:01 PDT
(In reply to comment #5)
> Comment on attachment 262967 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262967&action=review
> 
> > Source/JavaScriptCore/ftl/FTLJITCode.h:89
> > +    SegmentedVector<OSRExitDescriptor, 8> osrExitDescriptors;
> 
> This should probably go to FTL::State since it's not used after
> FTL::compile().
Fil and I spoke over IRC: this is actually used after FTL::compile()
because each FTL::OSRExit has a reference to an item in this vector.
So I'm keeping it in FTL::JITCode.

> 
> > Source/JavaScriptCore/ftl/FTLStackMaps.h:129
> > -    typedef HashMap<uint32_t, Vector<Record>, WTF::IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> RecordMap;
> > +    struct RecordMapValue {
> > +        Vector<Record> records;
> > +        Vector<uint32_t> indices;
> > +    };
> > +    typedef HashMap<uint32_t, RecordMapValue, WTF::IntHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> RecordMap;
> 
> What about using Vector<RecordAndIndex> instead of RecordMapValue?  It's
> usually better to have one vector that contains a tuple, than a tuple
> containing multiple vectors.