Bug 219550 – [JIT] Require value registers explicitly on emitValueProfilingSite

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

We had a couple of bugs on baseline operations and forcing this parameter helps to avoid such kind of error on future changes.

[Caio Lima]

Created attachment 415449 [details]
Patch

[Yusuke Suzuki]

Can you take a look at debug build failure?

[Saam Barati]

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]

Created attachment 415551 [details]
Patch

[Caio Lima]

Created attachment 415552 [details]
Patch

[Caio Lima]

(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 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]

<rdar://problem/72051776>

[Caio Lima]

Created attachment 415632 [details]
Patch

[Caio Lima]

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]

Created attachment 415992 [details]
Patch

[EWS]

Committed r270711: <https://trac.webkit.org/changeset/270711>

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