Bug 237207 – [JSC] Reuse known register values on ARMv7

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Angelos Oikonomopoulos]

[JSC] Reuse known register values on ARMv7

[Angelos Oikonomopoulos]

Created attachment 453209 [details]
Patch

[Zan Dobersek]

Comment on attachment 453209 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:358
> -            DisallowMacroScratchRegisterUsage disallowScratch(jit);
>              jit.loadPtr(CCallHelpers::Address(callLinkInfoGPR, offsetOfCallee()), scratchGPR);
> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);

Is this necessary? In theory it shouldn't cause problems, but it feels safer to me to disallow usage for the whole block, load including.

[Angelos Oikonomopoulos]

(In reply to Zan Dobersek from comment #2)
> Comment on attachment 453209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453209&action=review
> 
> > Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:358
> > -            DisallowMacroScratchRegisterUsage disallowScratch(jit);
> >              jit.loadPtr(CCallHelpers::Address(callLinkInfoGPR, offsetOfCallee()), scratchGPR);
> > +            DisallowMacroScratchRegisterUsage disallowScratch(jit);
> 
> Is this necessary? In theory it shouldn't cause problems, but it feels safer
> to me to disallow usage for the whole block, load including.

It's necessary for the code to work in its current form, but not strictly necessary. The difference here is that loadPtr (indirectly) does RELEASE_ASSERT(m_allowScratchRegister) because it explicitly invalidates the associated CachedRegister by calling cachedAddressTempRegister().invalidate().

One could have all callers of load*() invalidate the destination register instead, but that felt a bit more error prone. Especially since my base tree didn't include any caller that would need to skip the invalidation (I discovered this new usage when rebasing on Friday). I'm open to changing things around if anyone thinks that would be more appropriate (either in theory or in practice).

[Zan Dobersek]

Comment on attachment 453209 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:358
>>> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);
>> 
>> Is this necessary? In theory it shouldn't cause problems, but it feels safer to me to disallow usage for the whole block, load including.
> 
> It's necessary for the code to work in its current form, but not strictly necessary. The difference here is that loadPtr (indirectly) does RELEASE_ASSERT(m_allowScratchRegister) because it explicitly invalidates the associated CachedRegister by calling cachedAddressTempRegister().invalidate().
> 
> One could have all callers of load*() invalidate the destination register instead, but that felt a bit more error prone. Especially since my base tree didn't include any caller that would need to skip the invalidation (I discovered this new usage when rebasing on Friday). I'm open to changing things around if anyone thinks that would be more appropriate (either in theory or in practice).

I think the change should be fine. It was actually me changing it to the current form in bug #236064. Even if the scratch register will be needed for address resolution, it will be used as the load destination at the very end of this operation, and it's mostly after where we want to actively disallow the usage.

[Angelos Oikonomopoulos]

(In reply to Zan Dobersek from comment #4)
> Comment on attachment 453209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453209&action=review
> 
> >>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:358
> >>> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);
> >> 
> >> Is this necessary? In theory it shouldn't cause problems, but it feels safer to me to disallow usage for the whole block, load including.
> > 
> > It's necessary for the code to work in its current form, but not strictly necessary. The difference here is that loadPtr (indirectly) does RELEASE_ASSERT(m_allowScratchRegister) because it explicitly invalidates the associated CachedRegister by calling cachedAddressTempRegister().invalidate().
> > 
> > One could have all callers of load*() invalidate the destination register instead, but that felt a bit more error prone. Especially since my base tree didn't include any caller that would need to skip the invalidation (I discovered this new usage when rebasing on Friday). I'm open to changing things around if anyone thinks that would be more appropriate (either in theory or in practice).
> 
> I think the change should be fine. It was actually me changing it to the
> current form in bug #236064. Even if the scratch register will be needed for
> address resolution, it will be used as the load destination at the very end
> of this operation, and it's mostly after where we want to actively disallow
> the usage.

Yah, I tracked down the commit that made this change ;-)

Still, I'm not 100% sure from the above which change you think should be fine. Would you rather I keep the loadPtr out of DisallowMacroScratchRegisterUsage (i.e. keep the submitted patch as is)?

[Zan Dobersek]

Keep the submitted patch as-is, with the Disallow struct moved after loadPtr, where it used to be.

[EWS]

Committed r290589 (247867@main): <https://commits.webkit.org/247867@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89548061>

[Angelos Oikonomopoulos]

(In reply to Zan Dobersek from comment #6)
> Keep the submitted patch as-is, with the Disallow struct moved after
> loadPtr, where it used to be.

See also: https://bugs.webkit.org/show_bug.cgi?id=237888.