Bug 212791

Summary: REGRESSION(r262523): Fix testb3
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Tadeu Zagallo
Reported 2020-06-04 16:38:26 PDT
...
Attachments
Patch (4.24 KB, patch)
2020-06-04 16:41 PDT, Tadeu Zagallo 		no flags
DetailsFormatted DiffDiff
Patch (1.53 KB, patch)
2020-06-04 17:06 PDT, Tadeu Zagallo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tadeu Zagallo
Comment 1 2020-06-04 16:41:00 PDT
Created attachment 401093 [details] Patch
Mark Lam
Comment 2 2020-06-04 16:47:57 PDT
Comment on attachment 401093 [details] Patch r=me
Yusuke Suzuki
Comment 3 2020-06-04 16:48:07 PDT
Comment on attachment 401093 [details] Patch Can you add FIXME with URL to enable these tests?
Saam Barati
Comment 4 2020-06-04 16:50:43 PDT
Comment on attachment 401093 [details] Patch Why not just turn on Options::useB3HoistLoopInvariantValues?
Tadeu Zagallo
Comment 5 2020-06-04 17:06:09 PDT
Created attachment 401097 [details] Patch
EWS
Comment 6 2020-06-05 18:26:21 PDT
Committed r262666: <https://trac.webkit.org/changeset/262666> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401097 [details].
Radar WebKit Bug Importer
Comment 7 2020-06-05 18:27:16 PDT
<rdar://problem/64054082>
Saam Barati
Comment 8 2020-06-07 21:04:03 PDT
Comment on attachment 401097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401097&action=review > Source/JavaScriptCore/b3/testb3_1.cpp:783 > + Options::useB3HoistLoopInvariantValues() = true; You probably want to set this back to false after running LICM tests, right? I guess thinking about it there is definitely some weird raciness here since these tests are all run on another thread. Makes me think the most comprehensive solution is to have a bit on procedure that determines if LICM runs.
Mark Lam
Comment 9 2020-06-07 21:30:00 PDT
Comment on attachment 401097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401097&action=review >> Source/JavaScriptCore/b3/testb3_1.cpp:783 >> + Options::useB3HoistLoopInvariantValues() = true; > > You probably want to set this back to false after running LICM tests, right? I guess thinking about it there is definitely some weird raciness here since these tests are all run on another thread. Makes me think the most comprehensive solution is to have a bit on procedure that determines if LICM runs. Good point. Maybe the tests should just turn LICM all the time instead of just here. Is there any reason why we won't want to do that?
Saam Barati
Comment 10 2020-06-08 13:00:37 PDT
(In reply to Mark Lam from comment #9) > Comment on attachment 401097 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401097&action=review > > >> Source/JavaScriptCore/b3/testb3_1.cpp:783 > >> + Options::useB3HoistLoopInvariantValues() = true; > > > > You probably want to set this back to false after running LICM tests, right? I guess thinking about it there is definitely some weird raciness here since these tests are all run on another thread. Makes me think the most comprehensive solution is to have a bit on procedure that determines if LICM runs. > > Good point. Maybe the tests should just turn LICM all the time instead of > just here. Is there any reason why we won't want to do that? Yeah but note that setting Options means you will affect other tests too since they’re run concurrently
Note You need to log in before you can comment on or make changes to this bug.