Bug 134015 - add ftl infrastructure to windows
Summary: add ftl infrastructure to windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Enhancement
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-17 22:58 PDT by Alex Christensen
Modified: 2014-06-18 16:44 PDT (History)
4 users (show)

See Also:


Attachments
Patch (42.22 KB, patch)
2014-06-17 23:18 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (536.47 KB, application/zip)
2014-06-18 01:26 PDT, Build Bot
no flags Details
Patch (39.09 KB, patch)
2014-06-18 16:36 PDT, Alex Christensen
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2014-06-17 22:58:14 PDT
This is part of why I want the win64 jit so bad.  I got the jit and dfg working with https://bugs.webkit.org/show_bug.cgi?id=130638 and I just got the ftl to compile and link.  libllvmForJSC compiles and mostly links.
Comment 1 Alex Christensen 2014-06-17 23:18:52 PDT
Created attachment 233288 [details]
Patch
Comment 2 Build Bot 2014-06-18 01:26:53 PDT
Comment on attachment 233288 [details]
Patch

Attachment 233288 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5327732833517568

New failing tests:
media/W3C/video/readyState/readyState_during_canplay.html
Comment 3 Build Bot 2014-06-18 01:26:55 PDT
Created attachment 233298 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 4 Alex Christensen 2014-06-18 09:57:41 PDT
The only possible behavior change on mac would be if std::nan("") is not the same as 0.0 / 0.0.
Comment 5 Filip Pizlo 2014-06-18 10:37:24 PDT
Comment on attachment 233288 [details]
Patch

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

> Source/JavaScriptCore/disassembler/LLVMDisassembler.cpp:57
> +        _snprintf(

Can we have one common #define or something in WTF so that we don't have to #if PLATFORM in so many places just for this function?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1439
> +                        right, m_out.constDouble(std::nan("")))));

We should use PNaN here. It's a JSC thing.
Comment 6 Alex Christensen 2014-06-18 16:36:47 PDT
Created attachment 233336 [details]
Patch
Comment 7 Alex Christensen 2014-06-18 16:38:07 PDT
(In reply to comment #5)
> (From update of attachment 233288 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233288&action=review
> 
> > Source/JavaScriptCore/disassembler/LLVMDisassembler.cpp:57
> > +        _snprintf(
> 
> Can we have one common #define or something in WTF so that we don't have to #if PLATFORM in so many places just for this function?
I'll just leave that out of this patch until I can get a chance to test it.  There's also an issue with _snprintf not always writing a null character at the end if the string is the exact length of the buffer.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1439
> > +                        right, m_out.constDouble(std::nan("")))));
> 
> We should use PNaN here. It's a JSC thing.
Done.
Comment 8 Filip Pizlo 2014-06-18 16:39:05 PDT
Comment on attachment 233336 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1439
> -                right, m_out.constDouble(0.0 / 0.0))));
> +                        right, m_out.constDouble(PNaN))));

I don't think it's right to change the indentation here.
Comment 9 Alex Christensen 2014-06-18 16:44:16 PDT
(In reply to comment #8)
> I don't think it's right to change the indentation here.
Style bot does, but I didn't change it in the commit.

http://trac.webkit.org/changeset/170130