1433432 - Inline Object.is in Ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[André Bargull [:anba]]

If we add inlining support for Object.is(...), users don't need to worry whether or not calling Object.is(...) is (much) slower than using the triple-equals operator.

Comment 1

[André Bargull [:anba]]

Attachment: bug1433432.patch

This patch adds inlining support for Object.is(...). Notable changes:

MCallOptimize.cpp:
- If possible, |Object.is| is optimized as |===| using the existing MCompare class.
- If that's not possible, the new MSameValue class is used.
- I've used |callInfo.argc() < 2| as the inlining guard instead of the more common exact arguments count matching |callInfo.argc() == 2| just in case users start to use patterns like:

    var isNull = Object.is.bind(null, null);
    var hasNullElements = array.some(isNull);

  because that'd call Object.is with more than two arguments. 

TypePolicy.{h,cpp}
- Added a separate type policy for MSameValue, because when lowered, only three cases are supported:
  - Both arguments are doubles.
  - Both arguments are values.
  - The first argument is a value, the second a double.
- This approach matches how MCompare optimises certain argument combinations: https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/js/src/jit/IonBuilder.cpp#5799-5808

IonBuilder.cpp:
- Adding the extra bool argument to IonBuilder::compareTrySpecialized(...) feels a bit clumsy. But I don't quite see how to improve this right now...

CodeGenerator.cpp
The code to check for negative zero was copied (including the comments!) from https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/js/src/jit/CodeGenerator.cpp#12242-12250. I know that this won't result in efficient machine code [1], but I didn't see any more applicable MacroAssembler methods for this problem. But maybe someone who actually knows Assembler can make this faster in a follow-up bug. :-)

[1] For example using MacroAssemblerX86Shared::branchNegativeZero(...) made µ-benchmarks ~20% faster, but branchNegativeZero(...) is only available when targeting X86.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8945761 [details] [diff] [review]
bug1433432.patch

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

Looks great, thanks!

Maybe we should extend jit-test/tests/basic/object-is.js (or add a new file) with some tests for at least the double cases that are inlined?

Please make sure that adding a subtle bug to the masm code results in a test failure somewhere, at least with jit_test.py --ion/--tbpl :)

::: js/src/jit/CodeGenerator.cpp
@@ +7594,5 @@
> +        // is -Infinity instead of Infinity.
> +        Label isNegInf;
> +        masm.loadConstantDouble(1.0, temp);
> +        masm.divDouble(left, temp);
> +        masm.branchDouble(Assembler::DoubleLessThan, temp, left, &isNegInf);

Please file a follow-up bug to use branchNegativeZero :)

::: js/src/jit/MCallOptimize.cpp
@@ +2422,5 @@
> +        // Specialize |Object.is(lhs, rhs)| as |lhs === rhs|.
> +        bool emitted = false;
> +        MOZ_TRY(compareTrySpecialized(&emitted, JSOP_STRICTEQ, left, right, false));
> +        if (!emitted)
> +            return InliningStatus_NotInlined;

Could we use MSameValue in this case too?

::: js/src/jit/shared/LIR-shared.h
@@ +3163,5 @@
> +    LSameValueV(const LBoxAllocation& left, const LAllocation& right, const LDefinition& temp1,
> +                const LDefinition& temp2)
> +    {
> +        setBoxOperand(LhsInput, left);
> +        setOperand(1, right);

BOX_PIECES instead of 1, so it works on 32-bit.

Comment 3

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> ::: js/src/jit/CodeGenerator.cpp
> @@ +7594,5 @@
> > +        // is -Infinity instead of Infinity.
> > +        Label isNegInf;
> > +        masm.loadConstantDouble(1.0, temp);
> > +        masm.divDouble(left, temp);
> > +        masm.branchDouble(Assembler::DoubleLessThan, temp, left, &isNegInf);
> 
> Please file a follow-up bug to use branchNegativeZero :)

I was hoping for better generated assembler than the one emitted when branchNegativeZero is used. :-)

For example this is what GCC emits for the equivalent in C++:
---
_ZL9SameValuedd:
	movq	%xmm0, %rax
	movq	%xmm1, %rcx
	cmpq	%rcx, %rax
	je	.L5
	ucomisd	%xmm0, %xmm0
	setp	%dl
	ucomisd	%xmm1, %xmm1
	setp	%al
	andl	%edx, %eax
	ret
.L5:
	movl	$1, %eax
	ret
---

Source:
---
#include <cstdint>
#include <cstdlib>

inline uint64_t
BitwiseCast(const double aFrom)
{
  union
  {
    double mFrom;
    uint64_t mTo;
  } u;
  u.mFrom = aFrom;
  return u.mTo;
}

[[gnu::noinline]] static bool
SameValue(double aValue1, double aValue2)
{
  return BitwiseCast(aValue1) == BitwiseCast(aValue2) || (aValue1 != aValue1 && aValue2 != aValue2);
}

int main(int argc, char** argv) {
  double d1 = std::atof(argv[0]);
  double d2 = std::atof(argv[1]);
  return int(SameValue(d1, d2));
}
---

But I don't know how difficult it is to implement this for each architecture and whether or not it's worthwhile to spend too much time improving this at the assembly level.


> 
> ::: js/src/jit/MCallOptimize.cpp
> @@ +2422,5 @@
> > +        // Specialize |Object.is(lhs, rhs)| as |lhs === rhs|.
> > +        bool emitted = false;
> > +        MOZ_TRY(compareTrySpecialized(&emitted, JSOP_STRICTEQ, left, right, false));
> > +        if (!emitted)
> > +            return InliningStatus_NotInlined;
> 
> Could we use MSameValue in this case too?

Yes, we can use MSameValue here, too. I assumed compareTrySpecialized() handles every case which ends up here and only added the if-check instead of an assertion for extra safety. But when I just went through the logic in MCompare::determineCompareType(), it turned out that (Value x Object) and (Value x Symbol) aren't supported by MCompare (all other unsupported cases from MCompare don't apply here). 

Do you think it makes sense to file a bug to add support for (Value x Object) and (Value x Symbol) in MCompare? Implementation-wise it shouldn't too hard (probably just a type-check and a pointer-comparison if the types match?).

Comment 4

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Maybe we should extend jit-test/tests/basic/object-is.js (or add a new file)
> with some tests for at least the double cases that are inlined?
> 
> Please make sure that adding a subtle bug to the masm code results in a test
> failure somewhere, at least with jit_test.py --ion/--tbpl :)

The original patch already had a subtle bug which was caught when I added a new test for the double inlining case (the comparison used DoubleNotEqual instead of DoubleNotEqualOrUnordered). :-)

Comment 5

[André Bargull [:anba]]

Attachment: bug1433432.patch

Updated per review comments:
- Added jit-test/tests/basic/object-is-inlined.js to test when Object.is is called with (Double|Int32|Value) x (Double|Int32|Value).
- Changed IonBuilder::inlineObjectIs() to emit MSameValue if compareTrySpecialized() failed.
- Fixed LSameValueV to use the correct operand index (BOX_PIECES instead of 1).

Fixed issues:
- Fixed the first comparison in CodeGenerator::emitSameValue() to use DoubleNotEqualOrUnordered instead of DoubleNotEqual.
- Also changed the second comparison from DoubleNotEqualOrUnordered to DoubleNotEqual, because the second comparison is never executed for NaN values, and using DoubleNotEqual instead of DoubleNotEqualOrUnordered seems to be preferable if I understand this comment correctly: https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/js/src/jit/x86-shared/Assembler-x86-shared.h#317-319,334

Carrying r+.

Comment 6

[André Bargull [:anba]]

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

Comment 7

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Please file a follow-up bug to use branchNegativeZero :)

Filed bug 1434650.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/269c43e9132e
Inline Object.is(...) in Ion. r=jandem

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/269c43e9132e