Bug 137521

Summary: [WinCairo] Enable JIT on 32-bit.
Product: WebKit Reporter: peavo
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.christensen, benjamin, bfulgham, cmarcelo, commit-queue, mark.lam, msaboff
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description peavo 2014-10-08 06:41:06 PDT
I believe we can enable JIT, since the requirement to run on older CPUs (pre SSE2) is not that important for WinCairo.
Comment 1 peavo 2014-10-08 07:05:07 PDT
Created attachment 239472 [details]
Patch
Comment 2 Brent Fulgham 2014-10-08 09:39:08 PDT
This seems fine to me; it won't affect the Apple Windows port.
Comment 3 Alex Christensen 2014-10-08 09:59:44 PDT
Did the jit used to require sse2? Wasn't the jit enabled before? Couldn't we check to see if the CPU supports sse2 and enable it at runtime? Shouldn't the apple port do this, too? I don't think this is a good idea
Comment 4 Michael Saboff 2014-10-08 10:21:34 PDT
(In reply to comment #3)
> Did the jit used to require sse2? Wasn't the jit enabled before? Couldn't we check to see if the CPU supports sse2 and enable it at runtime? Shouldn't the apple port do this, too? I don't think this is a good idea

The LLInt and JITs use the SSE2 instructions for FP calculations.  We could check at runtime (patch welcome), but this should only be on a 32 bit path.

The Apple port doesn't need to check, all Intel Macs have new enough CPUs.
Comment 5 peavo 2014-10-08 10:35:41 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Did the jit used to require sse2? Wasn't the jit enabled before? Couldn't we check to see if the CPU supports sse2 and enable it at runtime? Shouldn't the apple port do this, too? I don't think this is a good idea
> 
> The LLInt and JITs use the SSE2 instructions for FP calculations.  We could check at runtime (patch welcome), but this should only be on a 32 bit path.
> 
> The Apple port doesn't need to check, all Intel Macs have new enough CPUs.

Would it be sufficient to do the same as in http://trac.webkit.org/changeset/167061, only adding a SSE2 check?

I searched through the code generated by the LLInt, and didn't find any SSE2 instructions, only x87 floating point instructions, as far as I could see.
Comment 6 Michael Saboff 2014-10-08 10:58:49 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Did the jit used to require sse2? Wasn't the jit enabled before? Couldn't we check to see if the CPU supports sse2 and enable it at runtime? Shouldn't the apple port do this, too? I don't think this is a good idea
> > 
> > The LLInt and JITs use the SSE2 instructions for FP calculations.  We could check at runtime (patch welcome), but this should only be on a 32 bit path.
> > 
> > The Apple port doesn't need to check, all Intel Macs have new enough CPUs.
> 
> Would it be sufficient to do the same as in http://trac.webkit.org/changeset/167061, only adding a SSE2 check?

That would probably work.

> I searched through the code generated by the LLInt, and didn't find any SSE2 instructions, only x87 floating point instructions, as far as I could see.

My bad.  I don't do much with the X87 part of the offline assembler.  The JIT don't have the SSE / X87 switch.  They only have an SSE2 path.
Comment 7 peavo 2014-10-08 11:46:17 PDT
Created attachment 239483 [details]
Patch
Comment 8 Alex Christensen 2014-10-08 11:56:51 PDT
I don't think adding the llint to the 32bit wincairo port without it enabled in the 32bit AppleWin port would be worth the small performance gain for the large risk of someone breaking something and nobody noticing
Comment 9 peavo 2014-10-08 12:12:07 PDT
(In reply to comment #8)
> I don't think adding the llint to the 32bit wincairo port without it enabled in the 32bit AppleWin port would be worth the small performance gain for the large risk of someone breaking something and nobody noticing

In the latest patch there is no difference between AppleWin and WinCairo; both enable LLINT, but disable JIT runtime if SSE2 is not present.
Comment 10 Mark Lam 2014-10-08 12:25:26 PDT
Comment on attachment 239483 [details]
Patch

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

Looks good but please fix the ChangeLog comments.  Thanks.

> Source/JavaScriptCore/ChangeLog:8
> +        Enable JIT on Windows 32-bit, but disable it runtime if SSE2 is not present.

typo": disable it *at* runtime

> Source/WTF/ChangeLog:8
> +        Enable JIT on Windows 32-bit, but disable it runtime if SSE2 is not present.

ditto.  Please fix comment.
Comment 11 peavo 2014-10-08 12:32:29 PDT
Created attachment 239487 [details]
Patch
Comment 12 Mark Lam 2014-10-08 12:33:22 PDT
Comment on attachment 239487 [details]
Patch

r=me
Comment 13 peavo 2014-10-08 12:34:38 PDT
(In reply to comment #12)
> (From update of attachment 239487 [details])
> r=me

Thanks for your help and review, guys :)
Comment 14 WebKit Commit Bot 2014-10-08 13:16:49 PDT
Comment on attachment 239487 [details]
Patch

Clearing flags on attachment: 239487

Committed r174473: <http://trac.webkit.org/changeset/174473>
Comment 15 WebKit Commit Bot 2014-10-08 13:16:54 PDT
All reviewed patches have been landed.  Closing bug.