Bug 144317

Summary: DFG+FTL should generate efficient code for branching on a string's boolean value.
Product: WebKit Reporter: Andreas Kling <kling>
Component: JavaScriptCoreAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, kling
Priority: P2 Keywords: Performance
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch fpizlo: review+

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>