Bug 150712 - The BinarySnippetRegisterContext needs to copy the result back from the scratch register.
Summary: The BinarySnippetRegisterContext needs to copy the result back from the scrat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 150936
Blocks: 150562
  Show dependency treegraph
 
Reported: 2015-10-29 22:20 PDT by Mark Lam
Modified: 2015-11-11 23:18 PST (History)
7 users (show)

See Also:


Attachments
the patch. (8.03 KB, patch)
2015-11-02 00:23 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff
patch 2: added optimization to support aliased operands and result registers. (10.06 KB, patch)
2015-11-02 15:35 PST, Mark Lam
no flags Details | Formatted Diff | Diff
svn up'ed (9.73 KB, patch)
2015-11-02 15:37 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.