Bug 212385 – Add some new emitters to the X86_64 and ARM64 MacroAssemblers.

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

[Mark Lam]

Specifically, these:
    clearBit64
    clearBits64WithMask
    countTrailingZeros64WithoutNullCheck

clearBit64 provides a faster way to clear a bit without having to a shift to make a mask first.
clearBits64WithMask does the equivalent of and64 with the 1's complement of the provided mask.
countTrailingZeros64WithoutNullCheck does the same thing as countTrailingZeros64, except that it assumes that the word in the register it is processing will never be null, and therefore skips the null check.  This is useful in code that already does a null check ahead of time.  So, there's no need to do a redundant null check.

[Mark Lam]

(In reply to Mark Lam from comment #0)
> clearBit64 provides a faster way to clear a bit without having to a shift to
> make a mask first.

Correction: clearBit64 will emit the shift to make the mask.

[Mark Lam]

Created attachment 400277 [details]
proposed patch.

[Radar WebKit Bug Importer]

<rdar://problem/63642626>

[Saam Barati]

Comment on attachment 400277 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:16
> +        clearBits64WithMask does the equivalent of and64 with the 1's complement of the
> +        provided mask.

feels weird to call this a "mask" when we're really using ~mask

[Mark Lam]

Comment on attachment 400277 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:16
>> +        provided mask.
> 
> feels weird to call this a "mask" when we're really using ~mask

It is a mask: one that defines the bits to clear instead of the bits to keep (as is the case with and64).  I'm open to an alternate term if you can think of anything better.

[Robin Morisset]

Comment on attachment 400277 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:22
> +        redundant null check.

Do we have any way of easily inserting an assert in the generated code when in debug? It could be useful for these kind of things.

> Source/JavaScriptCore/assembler/testmasm.cpp:474
> +    auto test = compile([=] (CCallHelpers& jit) {

what are you capturing by copy and why?

[Mark Lam]

Comment on attachment 400277 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:22
>> +        redundant null check.
> 
> Do we have any way of easily inserting an assert in the generated code when in debug? It could be useful for these kind of things.

Hmmm,  I'll see if it's easy to do.  If not, I'll do it in another patch.

>> Source/JavaScriptCore/assembler/testmasm.cpp:474
>> +    auto test = compile([=] (CCallHelpers& jit) {
> 
> what are you capturing by copy and why?

Copy paste error.  I'll remove the '='.

> Source/JavaScriptCore/assembler/testmasm.cpp:501
> +    auto test = compile([=] (CCallHelpers& jit) {

Ditto.

> Source/JavaScriptCore/assembler/testmasm.cpp:555
> +    auto test = compile([=] (CCallHelpers& jit) {

Ditto.

[Mark Lam]

(In reply to Mark Lam from comment #7)
> >> Source/JavaScriptCore/ChangeLog:22
> >> +        redundant null check.
> > 
> > Do we have any way of easily inserting an assert in the generated code when in debug? It could be useful for these kind of things.
> 
> Hmmm,  I'll see if it's easy to do.  If not, I'll do it in another patch.

I've added an assertion check.

[Mark Lam]

Created attachment 400284 [details]
patch for landing.

Thanks for the review.  Here's the updated patch for landing.

[Robin Morisset]

(In reply to Mark Lam from comment #8)
> (In reply to Mark Lam from comment #7)
> > >> Source/JavaScriptCore/ChangeLog:22
> > >> +        redundant null check.
> > > 
> > > Do we have any way of easily inserting an assert in the generated code when in debug? It could be useful for these kind of things.
> > 
> > Hmmm,  I'll see if it's easy to do.  If not, I'll do it in another patch.
> 
> I've added an assertion check.

Great, thanks!

[Mark Lam]

Created attachment 400285 [details]
patch for landing.

Also added the assertion to MacroAssemblerARM64 as well.  ARM64 doesn't really need the check because it's implementation will always have correctly even if the word is 0.  However, it's good to have the assertion consistently to help catch logic errors in the client that break this invariant.

[Mark Lam]

Landed in r262168: <http://trac.webkit.org/r262168>.