Bug 144317 - DFG+FTL should generate efficient code for branching on a string's boolean value.
Summary: DFG+FTL should generate efficient code for branching on a string's boolean va...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2015-04-28 00:09 PDT by Andreas Kling
Modified: 2015-04-28 11:55 PDT (History)
3 users (show)

See Also:

Attachments
Proposed patch (6.65 KB, patch)
2015-04-28 00:17 PDT, Andreas Kling 		fpizlo: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2015-04-28 00:09:36 PDT 
We shouldn't drop out to operationConvertJSValueToBoolean if we know the value to be a string.
Comment 1 Andreas Kling 2015-04-28 00:17:41 PDT 
Created attachment 251830 [details]
Proposed patch

All right, let's see if I got this right.
Comment 2 Geoffrey Garen 2015-04-28 11:01:10 PDT 
Comment on attachment 251830 [details]
Proposed patch

r=me

Can has FTL?
Comment 3 Filip Pizlo 2015-04-28 11:02:56 PDT 
Comment on attachment 251830 [details]
Proposed patch

This needs to have the FTL implementation as well.  Otherwise you're actually reducing FTL coverage with this patch.

So, please implement both DFG and FTL functionality in the same patch.
Comment 4 Andreas Kling 2015-04-28 11:14:29 PDT 
(In reply to comment #3)
> Comment on attachment 251830 [details]
> Proposed patch
> 
> This needs to have the FTL implementation as well.  Otherwise you're
> actually reducing FTL coverage with this patch.
> 
> So, please implement both DFG and FTL functionality in the same patch.

FTL already uses boolify() to implement compileBranch(). boolify() knows how to handle StringUse and generates nice code for it. Doesn't this mean that the FTL support is already in-place?
Comment 5 Filip Pizlo 2015-04-28 11:15:31 PDT 
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 251830 [details]
> > Proposed patch
> > 
> > This needs to have the FTL implementation as well.  Otherwise you're
> > actually reducing FTL coverage with this patch.
> > 
> > So, please implement both DFG and FTL functionality in the same patch.
> 
> FTL already uses boolify() to implement compileBranch(). boolify() knows how
> to handle StringUse and generates nice code for it. Doesn't this mean that
> the FTL support is already in-place?

Oh!  That's true.
Comment 6 Filip Pizlo 2015-04-28 11:16:13 PDT 
Comment on attachment 251830 [details]
Proposed patch

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

> LayoutTests/ChangeLog:3
> +        DFG should generate efficient code for branching on a string's boolean value.

Please make sure you note that this is about both DFG and FTL.  Either change the bug title, or put some blurb in the changelog.
Comment 7 Andreas Kling 2015-04-28 11:55:28 PDT 
Committed r183495: <http://trac.webkit.org/changeset/183495>