Bug 212791

Summary:

REGRESSION(r262523): Fix testb3

Product:

WebKit

Reporter:

Tadeu Zagallo <tzagallo>

Component:

JavaScriptCore

Assignee:

Tadeu Zagallo <tzagallo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

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