Bug 149970 – FTL should generate a unique OSR exit for each duplicated OSR exit stackmap intrinsic.

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 262967 [details]
patch

[WebKit Commit Bot]

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.

[Mark Lam]

Oops ... I closed the wrong bug.  Re-opening.

[Mark Lam]

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

[Filip Pizlo]

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.

[Saam Barati]

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

[Saam Barati]

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