Bug 188811 - [MSVC] Stop disabling /O2 features.
Summary: [MSVC] Stop disabling /O2 features.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-21 13:43 PDT by Ross Kirsling
Modified: 2018-08-22 15:24 PDT (History)
10 users (show)

See Also:

Attachments
Patch (1.66 KB, patch)
2018-08-21 13:46 PDT, Ross Kirsling 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.89 MB, application/zip)
2018-08-22 08:14 PDT, EWS Watchlist 		no flags Details
Patch (1.22 KB, patch)
2018-08-22 13:34 PDT, Ross Kirsling 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) 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 Ross Kirsling 2018-08-21 13:43:56 PDT 
[MSVC] Stop disabling /O2 features.
Comment 1 Ross Kirsling 2018-08-21 13:46:09 PDT 
Created attachment 347686 [details]
Patch
Comment 2 Ross Kirsling 2018-08-21 13:52:33 PDT 
/O2 should equate to all of these:
/Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy

But we have a few disabled:
/Oy- /GF- /Gy-

These appear to have been necessary for AppleWin back at the time of https://trac.webkit.org/changeset/190361.

But WinCairo doesn't seem to need to disable these optimizations, and I'm hoping the same is true for AppleWin.

This patch fixes bug 187737 as a side effect.
Comment 3 Ross Kirsling 2018-08-21 19:50:37 PDT 
Given that the AppleWin build passed and this doesn't seem to be making the existing test failures any worse, I'd say we're safe as hoped! :D
Comment 4 EWS Watchlist 2018-08-22 08:14:03 PDT 
Comment on attachment 347686 [details]
Patch

Attachment 347686 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8943713

New failing tests:
http/tests/security/local-video-source-from-remote.html
Comment 5 EWS Watchlist 2018-08-22 08:14:15 PDT 
Created attachment 347793 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 6 Per Arne Vollan 2018-08-22 13:11:46 PDT 
I seem to remember that JSC needs the frame pointer. Do you see any JSC test regressions when applying this patch?
Comment 7 Ross Kirsling 2018-08-22 13:26:35 PDT 
(In reply to Per Arne Vollan from comment #6)
> I seem to remember that JSC needs the frame pointer. Do you see any JSC test
> regressions when applying this patch?

Ahh -- I was hoping any issues would be made evident by Layout/API/Bindings/Built-ins tests, as we've yet to fully stabilize our JSC testing for WinCairo. (Actually in the past, JIT issues have often been reproable just by running MiniBrowser on a couple of pages.) If there are certain edge cases that require run-javascriptcore-tests to reliably encounter, would it be possible to have somebody test this with AppleWin?

Otherwise I suppose we could try it, keep a close eye on the AppleWin Debug Tests bot, and revert that part if things go unexpectedly south?
Comment 8 Ross Kirsling 2018-08-22 13:34:47 PDT 
Created attachment 347838 [details]
Patch
Comment 9 Per Arne Vollan 2018-08-22 13:54:50 PDT 
Comment on attachment 347838 [details]
Patch

R=me. Before landing, please check that changing these settings does not make the WebKit build significantly slower (both debug and release).
Comment 10 WebKit Commit Bot 2018-08-22 15:23:58 PDT 
Comment on attachment 347838 [details]
Patch

Clearing flags on attachment: 347838

Committed r235203: <https://trac.webkit.org/changeset/235203>
Comment 11 WebKit Commit Bot 2018-08-22 15:24:00 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-08-22 15:24:24 PDT 
<rdar://problem/43623013>