Bug 185106 – Macro assembler micro-optimizations for x86-64 and ARM64

• Status: NEW
• Component: JavaScriptCore

[JF Bastien]

I noticed a bunch of these while working on bug #184187. This patch only has the micro-optimizations, and a few annoying missing permutations of existing macro-ops. Most optimizations avoid using an extra register, use smaller immediates, smaller instruction encodings, etc.

[JF Bastien]

Created attachment 339056 [details]
patch

[JF Bastien]

Created attachment 339058 [details]
patch

Rebase.

[Filip Pizlo]

Comment on attachment 339058 [details]
patch

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

Please remove that assertion.

> Source/JavaScriptCore/assembler/MacroAssembler.h:370
> +        ASSERT((imm.m_value & 31) == imm.m_value);

This is not a valid assertion. It’s fine to call shift with an immediate that violates this assertion.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:283
> +        RegisterID scratch = src != dest ? dest : getCachedDataTempRegisterIDAndInvalidate();

How is this better?  I would revert this change unless you had some great evidence.

[Mark Lam]

Comment on attachment 339056 [details]
patch

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

> Source/JavaScriptCore/assembler/MacroAssembler.h:992
> +    void andPtr(RegisterID src, TrustedImm32 imm, RegisterID dest)
> +    {
> +        and64(imm, src, dest);
> +    }

Why have this but not the TrustedImm64 version?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:929
> +        move(TrustedImm64(-immediate), scratch);

Hmmm, I think this is wrong when imm == LLONG_MIN.

Let's say imm == LLONG_MIN and src == 1.  Then dest = LLONG_MIN - 1 ==> LLONG_MAX (with underflow).  This code here produces scratch == -LLONG_MIN ==> LLONG_MIN, and the result is LLONG_MIN + 1, which does not equal LLONG_MAX.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:3976
> -
> +    

Please undo.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:420
> +    

Undo blank space.

> Source/JavaScriptCore/assembler/X86Assembler.h:3622
> -
> +    

Please undo.

[Mark Lam]

Comment on attachment 339056 [details]
patch

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

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:929
>> +        move(TrustedImm64(-immediate), scratch);
> 
> Hmmm, I think this is wrong when imm == LLONG_MIN.
> 
> Let's say imm == LLONG_MIN and src == 1.  Then dest = LLONG_MIN - 1 ==> LLONG_MAX (with underflow).  This code here produces scratch == -LLONG_MIN ==> LLONG_MIN, and the result is LLONG_MIN + 1, which does not equal LLONG_MAX.

Sorry.  I mistaken swapped the arguments: the expected result (where scratch is 1) is 1 - LLONG_MIN, not LLONG_MIN - 1.   So, there's no issue here.

[JF Bastien]

Created attachment 339149 [details]
patch

Address whitespace issues.


(In reply to Filip Pizlo from comment #3)
> Comment on attachment 339058 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339058&action=review
> 
> Please remove that assertion.
> 
> > Source/JavaScriptCore/assembler/MacroAssembler.h:370
> > +        ASSERT((imm.m_value & 31) == imm.m_value);
> 
> This is not a valid assertion. It’s fine to call shift with an immediate
> that violates this assertion.

Removed.

I find this surprising though: wouldn't that mean that we've knowingly propagated an out-of-range value? I'd have expected us to hold it in range as an invariant, otherwise all the code that reasons about shifts with know RHS has to reason with a mask (not hard, but bug-prone, which this assert would catch). In practice that's immaterial for x86 and ARM64, but on ARM32 it means intermediate optimizations can get the wrong result because ARM32 masks the RHS with 0xFF. That would give the wrong JS result.


> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:283
> > +        RegisterID scratch = src != dest ? dest : getCachedDataTempRegisterIDAndInvalidate();
> 
> How is this better?  I would revert this change unless you had some great
> evidence.

Sorry the description I have is bad: this is better because you don't need to allow-temp. It's not an optimization, it's about correctness and ease of use. I've tripped on that bug before, and it seems dangerous because the "can I use temp-registers" assertion is debug-only, so code that seems right and passes all tests can still fail at runtime by clobbering a register unexpectedly. This change makes it easier to avoid this pitfall (pass src != dest), and less likely that we hit a silent register clobber.

Would you like me to:

 1) Drop this entirely.
 2) Remove from this patch, put in a separate one with a better description.
 3) Keep here, and fix this patch's description.

[JF Bastien]

Created attachment 339206 [details]
patch

Fix 32-bit build.

[EWS Watchlist]

Comment on attachment 339206 [details]
patch

Attachment 339206 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7526734

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 339217 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[JF Bastien]

Created attachment 339239 [details]
patch

I discussed the dataTemp thing offline with Fil, and removed it for now since it's not a micro-optimization as the rest of the patch is.

[Mark Lam]

Comment on attachment 339239 [details]
patch

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

I think you have bugs.  Please re-check.

> Source/JavaScriptCore/assembler/MacroAssembler.h:418
> -
> +    

Please undo this insertion of blank spaces.

> Source/JavaScriptCore/assembler/MacroAssembler.h:928
> -
> +    

Blank space.

> Source/JavaScriptCore/assembler/MacroAssembler.h:952
> +    void andPtr(TrustedImm64 imm, RegisterID srcDest)
> +    {
> +        and64(imm, srcDest, srcDest);
> +    }

nit: Why use the ternary for here?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:-620
> -        ASSERT(src != dataTempRegister);

This assert should stay.  We never want src to be dataTempRegister because we're going to overwrite dataTempRegister with imm below.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:881
> +        signExtend32ToPtr(TrustedImm32(-imm.m_value), getCachedDataTempRegisterIDAndInvalidate());
> +        m_assembler.add<64>(dest, src, dataTempRegister);

This is wrong: you're implementing dest = src - imm instead of dest = imm - src.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:894
> -
> +        

Blank space.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:911
> +        if (isUInt12(-immediate)) {
> +            m_assembler.add<64>(dest, src, UInt12(static_cast<int32_t>(-immediate)));
> +            return;
> +        }

This is wrong: you're implementing dest = src - imm instead dest = imm - src.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:915
>          if (isUInt12(immediate)) {
> -            m_assembler.sub<64>(dest, dest, UInt12(static_cast<int32_t>(immediate)));
> +            m_assembler.sub<64>(dest, src, UInt12(static_cast<int32_t>(immediate)));
> +            return;
> +        }

This is wrong: you're implementing dest = src - imm instead dest = imm - src.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:918
> +        move(TrustedImm64(-immediate), getCachedDataTempRegisterIDAndInvalidate());
> +        m_assembler.add<64>(dest, src, dataTempRegister);

This is wrong: you're implementing dest = src - imm instead dest = imm - src.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:933
> -
> +        

Blank space.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:3275
> +        RegisterID scratch = left != dest ? dest : getCachedDataTempRegisterIDAndInvalidate();

I thought we're not going to do this in this patch.  Am I mistaken?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:3302
> +        RegisterID scratch = left != dest ? dest : getCachedDataTempRegisterIDAndInvalidate();

Ditto.  Are we doing this?

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:2983
> +        m_assembler.cmpl_im(left.m_value, right.offset, right.base);

This is inconsistent with all the compares above.  In all the compares above, m_assembler appears to expect the operands to be ordered (right, left).  Here, you're ordering them (left, right).  I suspect there's a bug here.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:637
> +        m_assembler.imull_i32r(src, imm.m_value, dest);

This looks suspicious.  i32r says imm, reg to me, but you're passing reg, imm.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:797
> +        add64(TrustedImm32(-imm.m_value), src, dest);

This is wrong: you're implementing dest = src - imm, instead of dest = imm - src.

[Alex Christensen]

Comment on attachment 339239 [details]
patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.