Bug 150712

Created attachment 264565 [details] the patch. Still need to run some tests on ARM64 but I think this patch should be good.

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 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).

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.

Created attachment 264641 [details] svn up'ed

<rdar://problem/23413682>

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 on attachment 264641 [details] svn up'ed r=me

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