Bug 219550

Summary: [JIT] Require value registers explicitly on emitValueProfilingSite
Product: WebKit Reporter: Caio Lima <ticaiolima>
Component: JavaScriptCoreAssignee: Caio Lima <ticaiolima>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, smoley, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Caio Lima 2020-12-04 12:31:08 PST 
We had a couple of bugs on baseline operations and forcing this parameter helps to avoid such kind of error on future changes.
Comment 1 Caio Lima 2020-12-04 12:44:07 PST 
Created attachment 415449 [details]
Patch
Comment 2 Yusuke Suzuki 2020-12-04 16:33:42 PST 
Can you take a look at debug build failure?
Comment 3 Saam Barati 2020-12-04 17:41:43 PST 
Comment on attachment 415449 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415449&action=review

> Source/JavaScriptCore/jit/JITCall.cpp:47
> +    emitValueProfilingSite(bytecode.metadata(m_codeBlock), JSValueRegs(regT0));

can we have a version on 64-bit that just takes a GPRReg, and it constructs the JSValueRegs so not all call sites need to do this?

Alternatively, we can make the constructor implicit on 64-bit :-)
Comment 4 Caio Lima 2020-12-07 05:08:33 PST 
Created attachment 415551 [details]
Patch
Comment 5 Caio Lima 2020-12-07 05:10:22 PST 
Created attachment 415552 [details]
Patch
Comment 6 Caio Lima 2020-12-07 05:14:23 PST 
(In reply to Yusuke Suzuki from comment #2)
> Can you take a look at debug build failure?

Build seems quite unrelated, since it was a failure caused by a missing header on code that I didn't change. I trigged a re-run and it then passed.
Comment 7 Caio Lima 2020-12-07 05:21:25 PST 
Comment on attachment 415449 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415449&action=review

Thank you very much for the review.

>> Source/JavaScriptCore/jit/JITCall.cpp:47
>> +    emitValueProfilingSite(bytecode.metadata(m_codeBlock), JSValueRegs(regT0));
> 
> can we have a version on 64-bit that just takes a GPRReg, and it constructs the JSValueRegs so not all call sites need to do this?
> 
> Alternatively, we can make the constructor implicit on 64-bit :-)

I went with the first proposal, since I don't quite understand the motivation behind the choice to turn `JSValueRegs(GPRReg)` explicit. Do you know why we declared it this way?
Comment 8 Radar WebKit Bug Importer 2020-12-07 09:58:12 PST 
<rdar://problem/72051776>
Comment 9 Caio Lima 2020-12-08 06:31:58 PST 
Created attachment 415632 [details]
Patch
Comment 10 Caio Lima 2020-12-08 08:01:06 PST 
Comment on attachment 415632 [details]
Patch

I'm starting to believe that this patch is indeed inserting issues. I'll investigate it further.
Comment 11 Caio Lima 2020-12-11 07:14:56 PST 
Created attachment 415992 [details]
Patch
Comment 12 EWS 2020-12-11 14:57:40 PST 
Committed r270711: <https://trac.webkit.org/changeset/270711>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415992 [details].