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

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>