Bug 137521 – [WinCairo] Enable JIT on 32-bit.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

I believe we can enable JIT, since the requirement to run on older CPUs (pre SSE2) is not that important for WinCairo.

Created attachment 239472 [details]
Patch

[Brent Fulgham]

This seems fine to me; it won't affect the Apple Windows port.

[Alex Christensen]

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

[Michael Saboff]

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

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

[Michael Saboff]

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

Created attachment 239483 [details]
Patch

[Alex Christensen]

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

[Mark Lam]

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.

Created attachment 239487 [details]
Patch

[Mark Lam]

Comment on attachment 239487 [details]
Patch

r=me

(In reply to comment #12)
> (From update of attachment 239487 [details])
> r=me

Thanks for your help and review, guys :)

[WebKit Commit Bot]

Comment on attachment 239487 [details]
Patch

Clearing flags on attachment: 239487

Committed r174473: <http://trac.webkit.org/changeset/174473>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.