Bug 150712

Summary: The BinarySnippetRegisterContext needs to copy the result back from the scratch register.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, keith_miller, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150936    
Bug Blocks: 150562    
Attachments:
Description Flags
the patch.
ggaren: review+
patch 2: added optimization to support aliased operands and result registers.
none
svn up'ed ggaren: review+

Description Mark Lam 2015-10-29 22:20:37 PDT
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.
Comment 1 Mark Lam 2015-11-02 00:23:58 PST
Created attachment 264565 [details]
the patch.

Still need to run some tests on ARM64 but I think this patch should be good.
Comment 2 Geoffrey Garen 2015-11-02 10:13:49 PST
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();
}
Comment 3 Mark Lam 2015-11-02 14:38:04 PST
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).
Comment 4 Mark Lam 2015-11-02 15:35:05 PST
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.
Comment 5 Mark Lam 2015-11-02 15:37:38 PST
Created attachment 264641 [details]
svn up'ed
Comment 6 Radar WebKit Bug Importer 2015-11-05 09:45:00 PST
<rdar://problem/23413682>
Comment 7 Mark Lam 2015-11-05 10:06:31 PST
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.
Comment 8 Geoffrey Garen 2015-11-06 17:23:01 PST
Comment on attachment 264641 [details]
svn up'ed

r=me
Comment 9 Mark Lam 2015-11-11 23:18:04 PST
Thanks.  Landed in r192353: <http://trac.webkit.org/r192353>.