Bug 154782 - FTL should lower its abstract heaps to B3 heap ranges
Summary: FTL should lower its abstract heaps to B3 heap ranges
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 154658
  Show dependency treegraph
 
Reported: 2016-02-27 16:34 PST by Filip Pizlo
Modified: 2016-02-28 13:07 PST (History)
5 users (show)

See Also:


Attachments
work in progress (30.75 KB, patch)
2016-02-27 16:38 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (33.31 KB, patch)
2016-02-27 20:11 PST, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-02-27 16:34:08 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2016-02-27 16:34:47 PST
This should unlock more load elimination in B3.  It's also essential for doing a good job on store elimination.
Comment 2 Filip Pizlo 2016-02-27 16:38:07 PST
Created attachment 272427 [details]
work in progress
Comment 3 Filip Pizlo 2016-02-27 20:11:07 PST
Created attachment 272435 [details]
the patch
Comment 4 WebKit Commit Bot 2016-02-27 20:13:30 PST
Attachment 272435 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:109:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:135:  JSCell_freeListNext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Saam Barati 2016-02-28 11:10:12 PST
Comment on attachment 272435 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:27
> +        unnecessary here sine we tightly control the height of the heap hierarchy.

typo: since

> Source/JavaScriptCore/b3/B3HeapRange.h:62
> +        // Detects overflow in debug mode. Ranges are compiler-controlled so this is not likely
> +        // to ever be an issue.

I don't think this comment is needed.

> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:87
> +        current = child->range().end();

Should this be taking the max() function? Or are children sorted?

> Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:201
> +    // It's super tempting to use lambdas for this, but I suspect that we'll create enough of
> +    // these that we want to do it the hard way.

I don't think this comment is needed.
Comment 6 Filip Pizlo 2016-02-28 12:32:28 PST
(In reply to comment #5)
> Comment on attachment 272435 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272435&action=review
> 
> > Source/JavaScriptCore/ChangeLog:27
> > +        unnecessary here sine we tightly control the height of the heap hierarchy.
> 
> typo: since

Fixed.

> 
> > Source/JavaScriptCore/b3/B3HeapRange.h:62
> > +        // Detects overflow in debug mode. Ranges are compiler-controlled so this is not likely
> > +        // to ever be an issue.
> 
> I don't think this comment is needed.

Removed.

> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:87
> > +        current = child->range().end();
> 
> Should this be taking the max() function? Or are children sorted?

No, and they're not sorted.  This line of code ensures its own correctness: the next iteration's call to compute() will take the last iteration's end.  The compute() method takes begin as an argument, and sets the range such that the end is greater than the begin.  Therefore, 'current' is always the current max.

> 
> > Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:201
> > +    // It's super tempting to use lambdas for this, but I suspect that we'll create enough of
> > +    // these that we want to do it the hard way.
> 
> I don't think this comment is needed.

Removed.
Comment 7 Filip Pizlo 2016-02-28 12:34:14 PST
Landed in http://trac.webkit.org/changeset/197299
Comment 8 Saam Barati 2016-02-28 13:07:38 PST
Comment on attachment 272435 [details]
the patch

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

>>> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:87
>>> +        current = child->range().end();
>> 
>> Should this be taking the max() function? Or are children sorted?
> 
> No, and they're not sorted.  This line of code ensures its own correctness: the next iteration's call to compute() will take the last iteration's end.  The compute() method takes begin as an argument, and sets the range such that the end is greater than the begin.  Therefore, 'current' is always the current max.

Right. Makes sense.