Bug 150712 – The BinarySnippetRegisterContext needs to copy the result back from the scratch register.

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

[Mark Lam]

If the BinarySnippetRegisterContext had re-assigned the result register to a scratch before emitting the snippet, it needs to copy the result back from the scratch after the snippet is done.

This fixes the cdjs-tests.yaml/main.js.ftl failure reported in https://bugs.webkit.org/show_bug.cgi?id=150687.

[Mark Lam]

Created attachment 264565 [details]
the patch.

Still need to run some tests on ARM64 but I think this patch should be good.

[Geoffrey Garen]

Comment on attachment 264565 [details]
the patch.

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

I'll say r=me because I think this code is an improvement, but I have some concerns.

> Source/JavaScriptCore/ftl/FTLCompile.cpp:350
>          if (!inputRegisters.get(m_result) && reservedRegisters.get(m_result))
>              m_result = m_allocator.allocateScratchGPR();

Please revert whitespace change.

I think I see a small bug or two here: If m_result *is* an input register and *is* reserved (add rNumberTag, rNumberTag, r0), we will not change it to a scratch register. Therefore, our snippet code will trample a reserved register (rNumberTag) when it writes the result.

Perhaps it is harmless to trample a reserved register when writing the result because, after writing the result, we will not execute any more MacroAssembler code that might depend on a reserved register. But if that is so, then we should never allocate a scratch register for the result.

So, either this code needs to be removed, or it needs to change to remove the inputRegisters condition.

Relatedly, if m_left is *equal to* m_right and *is* reserved (add r0, rNumberTag, rNumberTag), we will unnecessarily copy it twice. In both cases, I think we want code that says "If you have copied this register already, please set me to the copy". Something like:

if (reservedRegisters.get(m_right)) {
    if (m_right == m_origLeft)
        m_right = m_left;
    else
        m_right = m_allocator.allocateScratchGPR();
}

if (reservedRegisters.get(m_result)) {
    if (m_result == m_origLeft)
        m_result = m_left;
    else if (m_result == m_origRight)
        m_result = m_right;
    else
        m_result = m_allocator.allocateScratchGPR();
}

[Mark Lam]

Comment on attachment 264565 [details]
the patch.

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

>> Source/JavaScriptCore/ftl/FTLCompile.cpp:350
>>              m_result = m_allocator.allocateScratchGPR();
> 
> Please revert whitespace change.
> 
> I think I see a small bug or two here: If m_result *is* an input register and *is* reserved (add rNumberTag, rNumberTag, r0), we will not change it to a scratch register. Therefore, our snippet code will trample a reserved register (rNumberTag) when it writes the result.
> 
> Perhaps it is harmless to trample a reserved register when writing the result because, after writing the result, we will not execute any more MacroAssembler code that might depend on a reserved register. But if that is so, then we should never allocate a scratch register for the result.
> 
> So, either this code needs to be removed, or it needs to change to remove the inputRegisters condition.
> 
> Relatedly, if m_left is *equal to* m_right and *is* reserved (add r0, rNumberTag, rNumberTag), we will unnecessarily copy it twice. In both cases, I think we want code that says "If you have copied this register already, please set me to the copy". Something like:
> 
> if (reservedRegisters.get(m_right)) {
>     if (m_right == m_origLeft)
>         m_right = m_left;
>     else
>         m_right = m_allocator.allocateScratchGPR();
> }
> 
> if (reservedRegisters.get(m_result)) {
>     if (m_result == m_origLeft)
>         m_result = m_left;
>     else if (m_result == m_origRight)
>         m_result = m_right;
>     else
>         m_result = m_allocator.allocateScratchGPR();
> }

The last thing the snippets will do is to box the result.  Boxing may require the use of the TagTypeNumber register.  So, I'll stick with keeping the result separate from the reserved registers.

I'll update the BinarySnippetRegisterContext to handle cases where the operands and result may be aliases of the same register(s).

[Mark Lam]

Created attachment 264639 [details]
patch 2: added optimization to support aliased operands and result registers.

This patch has passed the JSC tests on X86_64.  However, on ARM64, I'm seeing some failures.  Now in the process of checking if those are pre-existing failures or if they are due to this patch.

[Mark Lam]

Created attachment 264641 [details]
svn up'ed

[Radar WebKit Bug Importer]

<rdar://problem/23413682>

[Mark Lam]

The failure on ARM64 is due to a latent bug which I'll investigate here: https://bugs.webkit.org/show_bug.cgi?id=150936.  I don't think that bug is related to the op_sub implementation at all, but rather due to the fact that we can now FTL compile a certain function. 

Hence, this patch is ready for a review.

[Geoffrey Garen]

Comment on attachment 264641 [details]
svn up'ed

r=me

[Mark Lam]

Thanks.  Landed in r192353: <http://trac.webkit.org/r192353>.