Bug 212791 - REGRESSION(r262523): Fix testb3
Summary: REGRESSION(r262523): Fix testb3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-04 16:38 PDT by Tadeu Zagallo
Modified: 2020-06-08 13:00 PDT (History)
7 users (show)

See Also:

Attachments
Patch (4.24 KB, patch)
2020-06-04 16:41 PDT, Tadeu Zagallo 		no flags Details | Formatted Diff | Diff
Patch (1.53 KB, patch)
2020-06-04 17:06 PDT, Tadeu Zagallo 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) 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 Tadeu Zagallo 2020-06-04 16:38:26 PDT 
...
Comment 1 Tadeu Zagallo 2020-06-04 16:41:00 PDT 
Created attachment 401093 [details]
Patch
Comment 2 Mark Lam 2020-06-04 16:47:57 PDT 
Comment on attachment 401093 [details]
Patch

r=me
Comment 3 Yusuke Suzuki 2020-06-04 16:48:07 PDT 
Comment on attachment 401093 [details]
Patch

Can you add FIXME with URL to enable these tests?
Comment 4 Saam Barati 2020-06-04 16:50:43 PDT 
Comment on attachment 401093 [details]
Patch

Why not just turn on Options::useB3HoistLoopInvariantValues?
Comment 5 Tadeu Zagallo 2020-06-04 17:06:09 PDT 
Created attachment 401097 [details]
Patch
Comment 6 EWS 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].
Comment 7 Radar WebKit Bug Importer 2020-06-05 18:27:16 PDT 
<rdar://problem/64054082>
Comment 8 Saam Barati 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.
Comment 9 Mark Lam 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?
Comment 10 Saam Barati 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