1481247 - MacroAssembler::compareStrings checks twice for pointer equivalence

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[André Bargull [:anba]]

MacroAssembler::compareStrings currently emits for x64:
---
[Codegen] instruction CompareS
[Codegen] cmpq       %r8, %rsi
[Codegen] jne        .Lfrom8017
[Codegen] movl       $0x1, %edi
[Codegen] jmp        .Lfrom8027
[Codegen] .set .Llabel8027, .
[Codegen] .set .Lfrom8017, .Llabel8027
[Codegen] testl      $0x1, 0x0(%rsi)
[Codegen] jne        .Lfrom8039
[Codegen] testl      $0x1, 0x0(%r8)
[Codegen] jne        .Lfrom8052
[Codegen] cmpq       %r8, %rsi
[Codegen] sete       %dil
[Codegen] movzbl     %dil, %edi
[Codegen] jmp        .Lfrom8068
[Codegen] .set .Llabel8068, .
[Codegen] .set .Lfrom8052, .Llabel8068
[Codegen] .set .Lfrom8039, .Llabel8068
[Codegen] movl       0x4(%rsi), %edi
[Codegen] cmpl       %edi, 0x4(%r8)
[Codegen] je         .Lfrom8081
[Codegen] xorl       %edi, %edi
[Codegen] .set .Llabel8083, .
[Codegen] .set .Lfrom8068, .Llabel8083
[Codegen] .set .Lfrom8027, .Llabel8083
[Codegen] .set .Llabel8083, .
---


But can be optimized to:
---
[Codegen] instruction CompareS
[Codegen] cmpq       %r8, %rsi
[Codegen] jne        .Lfrom8017
[Codegen] movl       $0x1, %edi
[Codegen] jmp        .Lfrom8027
[Codegen] .set .Llabel8027, .
[Codegen] .set .Lfrom8017, .Llabel8027
[Codegen] testl      $0x1, 0x0(%rsi)
[Codegen] jne        .Lfrom8039
[Codegen] testl      $0x1, 0x0(%r8)
[Codegen] je         .Lfrom8052
[Codegen] .set .Llabel8052, .
[Codegen] .set .Lfrom8039, .Llabel8052
[Codegen] movl       0x4(%rsi), %edi
[Codegen] cmpl       %edi, 0x4(%r8)
[Codegen] je         .Lfrom8065
[Codegen] .set .Llabel8065, .
[Codegen] .set .Lfrom8052, .Llabel8065
[Codegen] xorl       %edi, %edi
[Codegen] .set .Llabel8067, .
[Codegen] .set .Lfrom8027, .Llabel8067
[Codegen] .set .Llabel8067, .
---

Comment 1

[André Bargull [:anba]]

Attachment: bug1481247.patch

We've already checked for pointer equivalence here [1], which means the comparison at [2] is redundant.

[1] https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/js/src/jit/MacroAssembler.cpp#1433
[2] https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/js/src/jit/MacroAssembler.cpp#1445

Comment 2

[Ted Campbell [:tcampbell]]

Comment on attachment 8997973 [details] [diff] [review]
bug1481247.patch

Review of attachment 8997973 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find

::: js/src/jit/MacroAssembler.cpp
@@ +1428,5 @@
>      MOZ_ASSERT(IsEqualityOp(op));
>  
>      Label done;
>      Label notPointerEqual;
> +    // Pointer comparison to detect identical strings.

"Identical" seems to be ambiguous between pointer equality and content equality.

// If operands point to same JSString (or JSAtom) instance, set result for equalality.

@@ +1436,5 @@
>  
>      bind(&notPointerEqual);
>  
>      Label notAtom;
> +    Label bothAtom;

s/bothAtom/setNotEqualResult/

Let's describe the action instead

Comment 3

[André Bargull [:anba]]

Attachment: bug1481247.patch

(In reply to Ted Campbell [:tcampbell] from comment #2)
> > +    // Pointer comparison to detect identical strings.
> 
> "Identical" seems to be ambiguous between pointer equality and content
> equality.
> 
> // If operands point to same JSString (or JSAtom) instance, set result for
> equalality.
> 

Ended up with "If operands point to the same instance, the strings are trivially equal.", which still fits in 79 columns.

Carrying r+.

Comment 4

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b14365c3ab9574eab63e4dd096ad03c88dd915ee

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7311e3026a47
Don't compare pointers twice in MacroAssembler::compareStrings. r=tcampbell

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7311e3026a47