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

Caio Lima
Reported 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.
Attachments
Patch (30.79 KB, patch)
2020-12-04 12:44 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (31.54 KB, patch)
2020-12-07 05:08 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (31.54 KB, patch)
2020-12-07 05:10 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (31.52 KB, patch)
2020-12-08 06:31 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (31.53 KB, patch)
2020-12-11 07:14 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Caio Lima
Comment 1 2020-12-04 12:44:07 PST
Created attachment 415449 [details] Patch
Yusuke Suzuki
Comment 2 2020-12-04 16:33:42 PST
Can you take a look at debug build failure?
Saam Barati
Comment 3 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 :-)
Caio Lima
Comment 4 2020-12-07 05:08:33 PST
Created attachment 415551 [details] Patch
Caio Lima
Comment 5 2020-12-07 05:10:22 PST
Created attachment 415552 [details] Patch
Caio Lima
Comment 6 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.
Caio Lima
Comment 7 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?
Radar WebKit Bug Importer
Comment 8 2020-12-07 09:58:12 PST
<rdar://problem/72051776>
Caio Lima
Comment 9 2020-12-08 06:31:58 PST
Created attachment 415632 [details] Patch
Caio Lima
Comment 10 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.
Caio Lima
Comment 11 2020-12-11 07:14:56 PST
Created attachment 415992 [details] Patch
EWS
Comment 12 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].
Note You need to log in before you can comment on or make changes to this bug.