1434717 - Convert UnaryArith Shared Inline Cache to CacheIR

Status: RESOLVED FIXED

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

Description

[Matthew Gaudet (he/him) [:mgaudet]]

In order to simplify the architecture of the inline caches I'd like to slowly replace the shared IC system with CacheIR ICs. 

This bug tracks the work to replace the UnaryArith Shared IC with a CacheIR replacement.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 1: Implement GuardIsInt32

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 2: Implement CacheIR IC for unary arithmetic operators

This provides a CacheIR implementation of the IC for JSOP_BITNOT and JSOP_NEG.
However this commit does not connect the IC, as that requires more work to the
shared IC system.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 3: Switch to using CacheIR for Baseline ICs

As a side effect of this patch, UnaryArith loses it's ability to attach ICs in
Ion. That support will come in a subsequent patch.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 4: Remove UnsaryShared IC support

It appears this also removes dead remnents of a previous caching story for getprop.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 5: Initial progress on UnaryArith IC for IonMonkey

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Current status is that Part 5 is flawed: the IC is attached, but something goes wrong in patching, and we instead reliably jump to the bailout.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 1: Implement GuardIsInt32

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 2: Implement CacheIR IC for unary arithmetic operators

This provides a CacheIR implementation of the IC for JSOP_BITNOT and JSOP_NEG.
However this commit does not connect the IC, as that requires more work to the
shared IC system.

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 3: Switch to using CacheIR for Baseline ICs

As a side effect of this patch, UnaryArith loses it's ability to attach ICs in
Ion. That support will come in a subsequent patch.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 4: Remove UnsaryShared IC support

It appears this also removes dead remnents of a previous caching story for getprop.

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 5: Connect UnaryArith IC to IonMonkey

Comment 12

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 6: Implement UnaryArith IC for doubles

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 4: Remove UnaryShared IC support

It appears this also removes dead remnents of a previous caching story for getprop.

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 5: Connect UnaryArith IC to IonMonkey

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 6: Implement UnaryArith IC for doubles

Comment 16

[Matthew Gaudet (he/him) [:mgaudet]]

TryBuild: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1f8d46f8df3457e13c3897b2e11a6e4307e7d57

I did a 5x run of octane, comparing w/ and w/o these patches: I got a 0.12% regression (well within the noise of the benchmark), however I'm very open to help in identifying other suites I ought to run before landing this, as it does potentially use registers less effectively than the shared ICs.

Comment 17

[Matthew Gaudet (he/him) [:mgaudet]]

One other thing I should point out: Part 3: Breaks |BaselineInspector::expectedResultType()|, which could cause performance regressions. 

I'm not sure what a good way to fix that function is tho.

Comment 18

[Ted Campbell [:tcampbell]]

Comment on attachment 8949458 [details] [diff] [review]
Part 1: Implement GuardIsInt32

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

Makes sense.

::: js/src/jit/CacheIRCompiler.cpp
@@ +1316,5 @@
> +{
> +    ValOperandId inputId = reader.valOperandId();
> +    Register output = allocator.defineRegister(masm, reader.int32OperandId());
> +
> +    if (allocator.knownType(inputId) == JSVAL_TYPE_INT32) {

Ah, I did not know we could do this.

Comment 19

[Ted Campbell [:tcampbell]]

Comment on attachment 8949460 [details] [diff] [review]
Part 2: Implement CacheIR IC for unary arithmetic operators

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

Looks good. I'd like to see the version with something done about GuardInt32ZeroOrMinInt before I r+.

::: js/src/jit-test/tests/cacheir/unaryarith.js
@@ +14,5 @@
> +warmup(fun1, 1, -1);
> +warmup(fun1, 0, -0);
> +
> +warmup(fun2, 3, -3);
> +warmup(fun2, 1.2, -1.2);

Add the mixed int/double case even if not currently optimized, please.

::: js/src/jit/CacheIR.cpp
@@ +4953,5 @@
> +    return true;
> +}
> +
> +bool
> +UnaryArithIRGenerator::tryAttachNumber()

Leave this out of this patch.

::: js/src/jit/CacheIR.h
@@ +174,4 @@
>      _(GuardIsNumber)                      \
>      _(GuardIsInt32)                       \
>      _(GuardIsInt32Index)                  \
> +    _(GuardInt32ZeroOrMinInt)             \

I think we can absorb this into the Int32NegationResult.

@@ +275,4 @@
>      _(LoadStringResult)                   \
>      _(LoadInstanceOfObjectResult)         \
>      _(LoadTypeOfObjectResult)             \
> +    _(LoadInt32NotResult)                 \

My preference is to drop the Load prefix since so memory is involved anywhere.

Comment 20

[Ted Campbell [:tcampbell]]

Comment on attachment 8949461 [details] [diff] [review]
Part 3: Switch to using CacheIR for Baseline ICs

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

Looking great. I'd like to see it again once the nits are fixed.

Nit: watch for weird trailing whitespace in your files.

::: js/src/jit/BaselineIC.cpp
@@ +4573,5 @@
> +                                    op, val, res);
> +        if (gen.tryAttachStub()) {
> +            bool attached = false;
> +            ICStub* newStub = AttachBaselineCacheIRStub(cx, gen.writerRef(), gen.cacheKind(),
> +                                                        BaselineCacheIRStubKind::Regular, //Monitored?

Regular is correct here. While in general we use Monitored for things that return values, the concern is only if a new type could appear. This is the case for GetProp where we are reading an arbitrary user value for memory, but for the case of this stub we know the result is an Int32.

Looking at the opcode table, we see that JSOP_NEG/JSOP_NOT do not have the JOF_TYPESET flag. Only ops with this flag track the typeset of their results. If a typeset isn't tracked, Ion must either infer or assume the worst.

::: js/src/jit/BaselineIC.h
@@ +1489,5 @@
> +      protected:
> +        MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override;
> +
> +      public:
> +        explicit Compiler(JSContext* cx, Engine engine)

The 'engine' variable is dead now since the fallback stub is unshared, and cacheir handles the rest.

Comment 21

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 2: Implement CacheIR IC for unary arithmetic operators

This provides a CacheIR implementation of the IC for JSOP_BITNOT and JSOP_NEG.
However this commit does not connect the IC, as that requires more work to the
shared IC system.

Comment 22

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 2: Implement CacheIR IC for unary arithmetic operators

This provides a CacheIR implementation of the IC for JSOP_BITNOT and JSOP_NEG.
However this commit does not connect the IC, as that requires more work to the
shared IC system.

Comment 23

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 3: Switch to using CacheIR for Baseline ICs

As a side effect of this patch, UnaryArith loses it's ability to attach ICs in
Ion. That support will come in a subsequent patch.

Comment 24

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 4: Remove UnaryShared IC support

It appears this also removes dead remnents of a previous caching story for getprop.

Comment 25

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 5: Connect UnaryArith IC to IonMonkey

Comment 26

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 6: Implement UnaryArith IC for doubles

Comment 27

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8950735 [details] [diff] [review]
Part 4: Remove UnaryShared IC support

Carrying tcampbell's review; this patch was rebased.

Comment 28

[Matthew Gaudet (he/him) [:mgaudet]]

All the feedback has been addressed, with one exception: 

(In reply to Ted Campbell [:tcampbell] from comment #20)
> Comment on attachment 8949461 [details] [diff] [review]
> Part 3: Switch to using CacheIR for Baseline ICs
> 
> <snip> 
>
> The 'engine' variable is dead now since the fallback stub is unshared, and
> cacheir handles the rest.

It's not clear to me if this is the case. It seems this is still needed for generating the stub code?

Comment 29

[Matthew Gaudet (he/him) [:mgaudet]]

One other thing: This patch changes from LoadInt32NegationResult et. al. to Int32NegationResult. While I do think this is better, I'll have to open a followup bug to change the Load*TruthyResult ops for consistency.

Comment 30

[Jan de Mooij [:jandem]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #27)
> Part 4: Remove UnaryShared IC support

\o/ Once all shared stubs are gone, we can clean up more code.

Comment 31

[Ted Campbell [:tcampbell]]

Comment on attachment 8950732 [details] [diff] [review]
Part 2: Implement CacheIR IC for unary arithmetic operators

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

Nice and simple :)

::: js/src/jit/CacheIRCompiler.cpp
@@ +1811,5 @@
> +    FailurePath* failure;
> +    if (!addFailurePath(&failure))
> +        return false;
> +
> +    masm.branchTest32(Assembler::Zero, val, Imm32(0x7fffffff), failure->label());

Add back the comment. // Guard against 0 and MIN_INT, both result in a double.

Comment 32

[Ted Campbell [:tcampbell]]

Comment on attachment 8950733 [details] [diff] [review]
Part 3: Switch to using CacheIR for Baseline ICs

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

Looks good once we have the perf for Ion.

::: js/src/jit/IonBuilder.cpp
@@ +3543,5 @@
>      if (actualOp == JSOP_POS)
>          return Ok();
>  
> +    // JSOP_NEG can't be handled here right now.
> +    if (actualOp == JSOP_NEG)

I believe you need JSOP_BITNOT here as well, or the MOZ_CRASH below will get you.

Comment 33

[Ted Campbell [:tcampbell]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #28)
> All the feedback has been addressed, with one exception: 
> 
> (In reply to Ted Campbell [:tcampbell] from comment #20)
> > Comment on attachment 8949461 [details] [diff] [review]
> > Part 3: Switch to using CacheIR for Baseline ICs
> > 
> > <snip> 
> >
> > The 'engine' variable is dead now since the fallback stub is unshared, and
> > cacheir handles the rest.
> 
> It's not clear to me if this is the case. It seems this is still needed for
> generating the stub code?

I was thinking like this example: https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/js/src/jit/BaselineIC.h#405

Move the BaselineEngine constant inside since these fallback stubs are hard-coded for baseline.

Comment 34

[Ted Campbell [:tcampbell]]

Comment on attachment 8950736 [details] [diff] [review]
Part 5: Connect UnaryArith IC to IonMonkey

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

Excellent work.

::: js/src/jit/BaselineIC.cpp
@@ +4585,4 @@
>      return true;
>  }
>  
> +// static bool

junk comment

::: js/src/jit/CodeGenerator.cpp
@@ +362,5 @@
> +      case CacheKind::UnaryArith: {
> +        IonUnaryArithIC* unaryArithIC = ic->asUnaryArithIC();
> +
> +        saveLive(lir);
> +

Nit: extra newline

@@ +2627,5 @@
> +    LiveRegisterSet liveRegs = lir->safepoint()->liveRegs();
> +    TypedOrValueRegister input = TypedOrValueRegister(ToValue(lir, LUnaryCache::Input));
> +    ValueOperand output = GetValueOutput(lir);
> +
> +    IonUnaryArithIC ic(liveRegs,  input, output);

nit: whitespace

::: js/src/jit/IonBuilder.cpp
@@ +3281,4 @@
>              return Ok();
>      }
>  
> +    MOZ_TRY(arithTrySharedStub(&emitted, JSOP_BITNOT, nullptr, input));

We should probably rename this, but since you are working through all the Arith* SharedICs we can fix when you are done.

Comment 35

[Ted Campbell [:tcampbell]]

Comment on attachment 8950737 [details] [diff] [review]
Part 6: Implement UnaryArith IC for doubles

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

It all makes sense to me.

Comment 36

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 4: Remove UnaryShared IC support

It appears this also removes dead remnents of a previous caching story for getprop.

Comment 37

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 3: Switch to using CacheIR for Baseline ICs

As a side effect of this patch, UnaryArith loses it's ability to attach ICs in
Ion. That support will come in a subsequent patch.

Comment 38

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 2: Implement CacheIR IC for unary arithmetic operators

This provides a CacheIR implementation of the IC for JSOP_BITNOT and JSOP_NEG.
However this commit does not connect the IC, as that requires more work to the
shared IC system.

Comment 39

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 1: Implement GuardIsInt32

Comment 40

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 6: Implement UnaryArith IC for doubles

Comment 41

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 5: Connect UnaryArith IC to IonMonkey

Comment 42

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 1: Implement GuardIsInt32

Comment 43

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 2: Implement CacheIR IC for unary arithmetic operators

This provides a CacheIR implementation of the IC for JSOP_BITNOT and JSOP_NEG.
However this commit does not connect the IC, as that requires more work to the
shared IC system.

Comment 44

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 3: Switch to using CacheIR for Baseline ICs

As a side effect of this patch, UnaryArith loses it's ability to attach ICs in
Ion. That support will come in a subsequent patch.

Comment 45

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 4: Remove UnaryShared IC support

It appears this also removes dead remnents of a previous caching story for getprop.

Comment 46

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 5: Connect UnaryArith IC to IonMonkey

Comment 47

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 6: Implement UnaryArith IC for doubles

Comment 48

[Matthew Gaudet (he/him) [:mgaudet]]

Carrying tcampbell's r+ on the patch series.

Setting checkin-needed; here's the try build of these patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a718c58fa333e51df4550cf2e3a006d4a381c0ff (though, I've rebased on central now that Bug 1435569 landed on central)

Comment 49

[Cosmin Sabou [:CosminS]]

This failed to apply, please take a look at it. Thanks.

applying base-fc0fd288a8a9
patching file js/src/jit/CodeGenerator.h
Hunk #1 FAILED at 135
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit/CodeGenerator.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh base-fc0fd288a8a9

This happened on Part 4: Remove UnaryShared IC support.

Comment 50

[Cosmin Sabou [:CosminS]]

You only had to upload patches with you will change.

Comment 51

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 4: Remove UnaryShared IC support

It appears this also removes dead remnents of a previous caching story for getprop.

Comment 52

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 5: Connect UnaryArith IC to IonMonkey

Comment 53

[Matthew Gaudet (he/him) [:mgaudet]]

Uploaded the two merged patches. These should apply now.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd6eaf7fc97
Part 1: Implement GuardIsInt32 r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/43a875bf1c8a
Part 2: Implement CacheIR IC for unary arithmetic operators r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5ec856f0a6
Part 3: Switch to using CacheIR for Baseline ICs. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1d9d0ebbe7
Part 6: Implement UnaryArith IC for doubles r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/286c42a171d8
Part 4: Remove UnaryShared IC support r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/52f1dfd75ff7
Part 5: Connect UnaryArith IC to IonMonkey r=tcampbell

Comment 55

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7fd6eaf7fc97
https://hg.mozilla.org/mozilla-central/rev/43a875bf1c8a
https://hg.mozilla.org/mozilla-central/rev/5c5ec856f0a6
https://hg.mozilla.org/mozilla-central/rev/bf1d9d0ebbe7
https://hg.mozilla.org/mozilla-central/rev/286c42a171d8
https://hg.mozilla.org/mozilla-central/rev/52f1dfd75ff7

Comment 56

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Revert CacheIR UnaryArith Stubs

This patch reverts the following revisions:

 52f1dfd75ff7
 286c42a171d8
 bf1d9d0ebbe7
 5c5ec856f0a6
 43a875bf1c8a
 7fd6eaf7fc97

due to issues determined during fuzzing.

Comment 57

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout due to issues determined during fuzzing: https://hg.mozilla.org/integration/mozilla-inbound/rev/11e086a7d4634a6e8f33f288422233a93734d601

Comment 58

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 1: Implement GuardIsInt32

Comment 59

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 2: Implement CacheIR IC for unary arithmetic operators

This provides a CacheIR implementation of the IC for JSOP_BITNOT and JSOP_NEG.
However this commit does not connect the IC, as that requires more work to the
shared IC system.

Comment 60

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 3: Switch to using CacheIR for Baseline ICs

As a side effect of this patch, UnaryArith loses it's ability to attach ICs in
Ion. That support will come in a subsequent patch.

Comment 61

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 4: Remove UnaryShared IC support

It appears this also removes dead remnents of a previous caching story for getprop.

Comment 62

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 5: Connect UnaryArith IC to IonMonkey

Comment 63

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 6: Implement UnaryArith IC for doubles

Comment 64

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 7: Ensure registers are saved and restored properly surrounding an ABI call

This resolves the test case found in Bug 1439180.

Comment 65

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Part 7: Ensure registers are saved and restored properly surrounding an ABI call

This resolves the test case found in Bug 1439180.

Comment 66

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Rollup of attached 7 patches for fuzzing.

Comment 67

[Matthew Gaudet (he/him) [:mgaudet]]

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

Comment 68

[Matthew Gaudet (he/him) [:mgaudet]]

I am hoping to have this series of patches get fuzzed before we try landing again (*hides head in shame*). 

To make that easier, I've attached a rollup patch of the whole series. 

In terms of focus, this patch changes the implementation the Unary arithmetic inline caches in baseline and Ion, and so that would be the point of focus.

Comment 69

[Ted Campbell [:tcampbell]]

Comment on attachment 8952466 [details] [diff] [review]
Part 7: Ensure registers are saved and restored properly surrounding an ABI call

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

Oops. I missed that as well.

Comment 70

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #66)
> Created attachment 8952472 [details]
> Rollup of attached 7 patches for fuzzing.

Sorry for the delay. I was unable to apply this to m-c tip rev bd6e200b5a6b nor 2c000486eac466da6623e4d7f7f1fd4e318f60e8. May I know what m-c rev this applies to?

If it's not current tip, may we request for it to be rebased to the current tip rev?

Comment 71

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Rollup of attached 7 patches for fuzzing, apply on m-c b184be598740

Rebased patches and attached new rollup patch

Should apply to m-c b184be598740.

Comment 72

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8954377 [details]
Rollup of attached 7 patches for fuzzing, apply on m-c b184be598740

Cautiously saying feedback+ for now. Haven't seen much in 24 hours.

Comment 73

[Christian Holler (:decoder)]

I also didn't see any failures after a while of fuzzing.

Comment 74

[Matthew Gaudet (he/him) [:mgaudet]]

Thanks :) 

This will wait 'til after the soft freeze to get re-landed.

Comment 75

[Matthew Gaudet (he/him) [:mgaudet]]

(Parts 6+7 will be squashed before the final push)

Comment 76

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: unary_benchmark.js

I circled back to this to take a brief look at performance. 

I see small performance losses (close to noise thresholds) with the CacheIR stubs, with one exception being negation of 0, where I see a ~50-60%  slow down relative to the shared stubs.

I don't think it's a barrier to checkin, but it's worth noting. With CacheIR we have the option to add a specialized sub for zero if need be with relatively low effort.  

Matthews-MacBook-Pro:build_OPT.OBJ mgaudet (WITHOUT CACHEIR)$ ./dist/bin/js ../unary_benchmark.js
fun: (x) => { return -x; }  Performance: 1816.91796875
fun: (x) => { return -x; }  Performance: 918.966064453125
fun: (x) => { return -x; }  Performance: 1861.006103515625
fun: (x) => { return -x; }  Performance: 1884.408203125
fun: (x) => { return ~x; }  Performance: 1954.2490234375
fun: (x) => { return ~x; }  Performance: 3792.94189453125

Matthews-MacBook-Pro:build_OPT.OBJ mgaudet (WITH CACHEIR)$ ./dist/bin/js ../unary_benchmark.js
fun: (x) => { return -x; }  Performance: 1839.739990234375
fun: (x) => { return -x; }  Performance: 1432.51806640625 (55% slower)
fun: (x) => { return -x; }  Performance: 1866.1259765625
fun: (x) => { return -x; }  Performance: 1925.1630859375
fun: (x) => { return ~x; }  Performance: 1948.39697265625
fun: (x) => { return ~x; }  Performance: 3879.879150390625

Comment 77

[Ted Campbell [:tcampbell]]

Comment on attachment 8952460 [details] [diff] [review]
Part 2: Implement CacheIR IC for unary arithmetic operators

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +1809,5 @@
> +    FailurePath* failure;
> +    if (!addFailurePath(&failure))
> +        return false;
> +
> +    // Guard against 0 and MIN_INT, both result in a double.

Add a comment explaining this assembly trickery. I get confused each time I see it.

"Guard against 0 and MIN_INT by checking if low 31-bits are all zero. Both of these result in a double."

Comment 78

[Ted Campbell [:tcampbell]]

Comment on attachment 8952466 [details] [diff] [review]
Part 7: Ensure registers are saved and restored properly surrounding an ABI call

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +1876,4 @@
>      masm.jump(&doneTruncate);
>  
>      masm.bind(&truncateABICall);
> +    LiveRegisterSet save(GeneralRegisterSet::Volatile(), liveVolatileFloatRegs());

I believe we can |save.takeUnchecked(FloatReg0);| since it is not live after callWithABI. We already push/pop FloatReg0 to preserve our callers value too.

Comment 79

[Ted Campbell [:tcampbell]]

Comment on attachment 8952464 [details] [diff] [review]
Part 6: Implement UnaryArith IC for doubles

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

r+ with Part7 squashed on this

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e2429e6d59
Part 1: Implement GuardIsInt32 r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc976c990dc8
Part 2: Implement CacheIR IC for unary arithmetic operators r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a2c849b641
Part 3: Switch to using CacheIR for Baseline ICs. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/055a7f061041
Part 4: Remove UnaryShared IC support r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea25dec22fd0
Part 5: Connect UnaryArith IC to IonMonkey r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b45cdbc1a5
Part 6: Implement UnaryArith IC for doubles r=tcampbell

Comment 81

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f7e2429e6d59
https://hg.mozilla.org/mozilla-central/rev/cc976c990dc8
https://hg.mozilla.org/mozilla-central/rev/b1a2c849b641
https://hg.mozilla.org/mozilla-central/rev/055a7f061041
https://hg.mozilla.org/mozilla-central/rev/ea25dec22fd0
https://hg.mozilla.org/mozilla-central/rev/e7b45cdbc1a5