1438727 - Convert BinaryArith Shared Inline Cache to CacheIR

Status: RESOLVED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #1434717 +++

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 BinaryArith Shared IC with a CacheIR replacement.

Comment 1

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

Attachment: Add test case for binary arithmetic operations

Comment 2

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

Attachment: [Part 0] Add test case for binary arithmetic operations

Comment 3

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

Attachment: [Part 1] Implement a subset of JSOP_ADD in CacheIR

This patch adds both Ion and Baseline support for ADD when the arguments are
doubles or int32.

This is implmented as a strangler via the SharedIC, this falls back to the shared
IC if there's no attachment in CacheIR. This should allow preservation of performance
throughout.

Comment 4

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

Comment on attachment 8961807 [details] [diff] [review]
[Part 1] Implement a subset of JSOP_ADD in CacheIR

Looking for a little feedback on this patch; four particular points stand out as needing feedback: 

1. Are we OK with implementing this transition via the strangler-pattern as this patch does. This will allow a staged transition for the ICs. 

2.  Inside CacheRegisterAllocator::loadDouble, to load from the payload reg, there's an awkward dance required to ensure we get to a double. While this works, it feels like there has to be a better way! 

3. In emitDoubleAddResult I don't preserve FloatRegister{1,0}. Despite this IC also being called from Ion, I've yet to see a failure, but this doesn't seem right. At the same time, It's not clear to me where to put the preservation machinery: Consumers of loadDouble would be forced into a very awkward dance to deal with failure labels that seems inelegant, but it's hard to encapsulate the restoration logic statically otherwise. 

4. loadDouble can't handle any pushes happening before it in the case where something is on a value or payload stack; is there a general pattern we use to handle save and restore while allowing stack operations to continue working?

Comment 5

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

Attachment: [Part 2] Implement a subset of JSOP_SUB in CacheIR

Comment 6

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

Attachment: [Part 3] Implement ADD+SUB for Boolean + Double|Int32

Comment 7

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

Attachment: [Part 4] Implement CacheIR IC for bitwise operations on Int32s

(As well as booleans combined with int32s)

Comment 8

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

Attachment: [Part 5] Implement branchMul32 in MacroAssembler

Comment 9

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

Attachment: [Part 6] Implement JSOP_MUL in CacheIR

This requires building usedFixedRegister to handle the x86 register requirments
of multiply operations.

Comment 10

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

Attachment: [Part 6] Allow allocateFixedRegister to spill in order to acquire a fixed register

In some circumstances the fixed register that's desired may be available only after a spill.

Comment 11

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

Attachment: [Part 7] Implement JSOP_MUL in CacheIR

This requires building usedFixedRegister to handle the x86 register requirments
of multiply operations.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8961807 [details] [diff] [review]
[Part 1] Implement a subset of JSOP_ADD in CacheIR

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

Nice work. I think with the comments below this answers all your questions, let me know if there's more.

About saving FloatReg0/FloatReg1, one option is to add these as fixed temps to your LIR instruction, so we can clobber them without having to save/restore them in the IC code (we could then assert these registers are not in liveRegs in CacheIRCompiler, to double check).

::: js/src/jit/CacheIRCompiler.cpp
@@ +130,5 @@
> +            AutoScratchValueRegister reg(*this, masm);
> +            MOZ_ASSERT(loc.payloadType() == JSVAL_TYPE_DOUBLE);
> +            // MG:XXX: this is mildly awkward
> +            masm.tagValue(loc.payloadType(), loc.payloadReg(), reg);
> +            masm.ensureDouble(reg.get(), dest, failure);

Is this case reachable? In general doubles are not "tagged" like other types; every double is a valid Value in our current boxing format. See for instance: https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/js/src/jit/MacroAssembler-inl.h#801

We don't have a TypedOperandId for doubles (as far as I know) and I don't think we can get a double constant here, so then we only have the Value cases to worry about (the other cases can just MOZ_CRASH).

Note that ensureDouble also supports an Address as |source|. Ideally we wouldn't use AutoScratchValueRegister in this function, not sure if that's possible, but I think it is. Let me know if you need help with this.

Comment 13

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

Attachment: WIP: Fixed allocation of float registers

(In reply to Jan de Mooij [:jandem] from comment #12)
> About saving FloatReg0/FloatReg1, one option is to add these as fixed temps
> to your LIR instruction, so we can clobber them without having to
> save/restore them in the IC code (we could then assert these registers are
> not in liveRegs in CacheIRCompiler, to double check).

That's a really interesting idea; To be sure I understood what you meant, I started prototyping a bit. I've attached a patch that shows the direction I was headed. Now, this patch doesn't quite work, as we run into an assertion

   Assertion failure: !minimalBundle(bundle), at /Users/mgaudet/mozilla-unified/js/src/jit/BacktrackingAllocator.cpp:1372

However, I wanted to get your eyes on it before trying to figure out too much more what's going on. 

(In reply to Jan de Mooij [:jandem] from comment #12)
> Note that ensureDouble also supports an Address as |source|. Ideally we
> wouldn't use AutoScratchValueRegister in this function, not sure if that's
> possible, but I think it is. Let me know if you need help with this.

You're right I can definitely cut some of them. Locally I've simplified loadDouble to only handle the cases we'd expect to encounter (no more PayloadReg or PayloadStack). I think we'd still need AutoScratchValueRegister if we allow constants though, as we need somewhere to materialize the constant. 

ValueStack is also doable.

Comment 14

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

(Whoops -- you already mentioned the constant case. Ignore that)

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #13)
> However, I wanted to get your eyes on it before trying to figure out too
> much more what's going on. 

I think the problem is that you need to use LDefinition::DOUBLE instead of LDefinition::FLOAT32.

FloatReg0 == xmm0, and xmm0 is:

  static constexpr FloatRegister xmm0 = FloatRegister(X86Encoding::xmm0, FloatRegisters::Double);

Note how FloatRegister stores the type (Float32/Double/Simd128) in addition to the register code, so a Double FloatRegister will never satisfy the LDefinition::FLOAT32 requirement. The !minimalBundle assert is usually regalloc's way of saying "can't allocate this".

Comment 16

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

Attachment: [Part 9] Implement JSOP_DIV in CacheIR

Here's a broad sketch of what I believe the DIV implemntation looks like in CacheIR. I just want to make sure you're on board the peculiarities involved in hoisting platform specific code into CacheIRCompiler.cpp. 

This patch has passed a try build.

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8973866 [details] [diff] [review]
[Part 9] Implement JSOP_DIV in CacheIR

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

The approach makes sense to me, but I'd suggest using the MacroAssembler abstraction for this: we already have remainder32 so maybe add something with a similar name (I can't think of anything I really like..) and then implement it in MacroAssembler-x86-shared.cpp (or the *-inl.h file) and for the other platforms :)

Comment 18

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

Attachment: [Part 0] Add test case for binary arithmetic operations

Comment 19

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

Attachment: [Part 1] Implement a subset of JSOP_ADD in CacheIR

This patch adds both Ion and Baseline support for ADD when the arguments are
doubles or int32.

This is implmented as a strangler via the SharedIC, this falls back to the shared
IC if there's no attachment in CacheIR. This should allow preservation of performance
throughout.

To provide clobber safety to the float registers, this patch uses fixed temporaries on
LBinaryCache.

Comment 20

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

Attachment: [Part 2] Implement a subset of JSOP_SUB in CacheIR

Comment 21

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

Attachment: [Part 3] Implement ADD+SUB for Boolean + Double|Int32

Comment 22

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

Attachment: [Part 4] Implement CacheIR IC for bitwise operations on Int32s

(As well as booleans combined with int32s)

Comment 23

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

Attachment: [Part 5] Implement branchMul32 in MacroAssembler

Comment 24

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

Attachment: [Part 6] Allow allocateFixedRegister to spill in order to acquire a fixed register

In some circumstances the fixed register that's desired may be available only after a spill.

Comment 25

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

Attachment: [Part 7] Clarify restrictions on imul and remove un-needed restriction

Comment 26

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

Attachment: [Part 8] Implement JSOP_MUL in CacheIR

This requires building usedFixedRegister to handle the x86 register requirments
of multiply operations.

Comment 27

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

Attachment: [Part 9] Implement flexible{quotient,remainder}32

These MacroAssembler methods paper over the inflexibility remainder32 on some platforms
by ensuring values are saved and restored to make the computations as non-invasive
as possible.

Comment 28

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

Attachment: [Part 10] Implement JSOP_DIV in CacheIR

Comment 29

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

Attachment: [Part 11] Implement JSOP_MOD in CacheIR

Comment 30

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

Attachment: [Part 12] Handle Double+Int32 bitwise operations

Comment 31

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

Attachment: [Part 13] Support Double DIV and Double MOD

Comment 32

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

Attachment: [Part 14] Implement flexible shift macroassembler helpers

Comment 33

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

Attachment: [Part 15] Support shifts in CacheIR

Comment 34

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

Attachment: [Part 16] Expand IC coverage by reducing guards to attach ICs where appropriate

Comment 35

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

Attachment: [Part 17] Add String+String Concatenation to CacheIR

Comment 36

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

Attachment: Add StringObject Concat (Not working with Ion)

This patch is preliminary (duplication of helper routines) but causes Ion bailout 
failures for reasons I do not yet understand. Throwing it up for feedback in case 
there's something that stands out to fresher eyes than mine.

Comment 37

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

Try build w/ Dromaeo+Kraken results for patch stack 

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

Comment 38

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

Attachment: binary_benchmark.js

Microbenchmarking operations on OS/X x64 shows some degradation, some severe, and some small wins: 

Operations and Arguments        Percent Improvement (Higher is Better) 	
Add: Doubles	                  -5.6
Add: Int32 + Int32 Overflow	   1.5
Add: Int32 Boolean	          -0.8
Add: Type Change Int32 -> Double	   1.5
BitAnd Double w/ Int32	           0.1
BitAnd Int32	                   1.3
BitOr  Boolean w/ Int32	          -0.8
BitXOr Double+int32	          -1.8
BitXOr Int32	                   0.5
Div: Boolean w/ Int32	         -19.3
Div: Double	                   2.2
Div: Int32	                 -62.3
Div: Type change Int32 -> Double	 -31.5
Lsh Boolean w/ Int32	          -0.5
Lsh Int32	                  -1.1
Mod: Boolean w/ Int32	         -81.2
Mod: Boolean w/ Int32	          32.7
Mod: Double	                  -3.0
Mod: Int32	                  -0.3
Mod: Type change Int32 -> Double	  -1.1
Mul: Boolean	                  50.4
Mul: Doubles	                 -14.1
Mul: Int32+  Int32 Overflow	  -1.7
Mul: Type change Int32 -> Double	  -1.7
Rsh Int32	                   1.6
Rsh Int32	                   0.6
Sub Double	                   2.4
Sub Int32	                  -5.4
URsh Boolean Int32	           0.7
URsh Int32	                  -1.5

As suspected div+mod are particularly impacted. 

Results for 'Add: String Concat' and 'Add: String Object Concat' are elided: The former because we appear to be hitting an optimization somewhere in the VM (the times are unbelievably low), and the latter because the benchmarking wasn't done with that IC available in CacheIR.

Comment 39

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

(Methodology note: The above numbers are the average of five runs, using the switch DISABLE_CACHEIR_BINARY=1 to change IC system)

Comment 40

[Jan de Mooij [:jandem]]

Comment on attachment 8975068 [details] [diff] [review]
Add StringObject Concat (Not working with Ion)

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

Hm I've no idea why it crashes. As mentioned on IRC, it could be stack pointer imbalance (or maybe another register gets clobbered). I'd suggest putting a masm.breakpoint() right before you call the first IC stub and one at the point after we return, then compare the register state in gdb. The VM call code we need in IonCacheIRCompiler is fairly complicated so it's possible this patch triggers a bug somewhere.

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +2688,5 @@
> +    AutoOutputRegister output(*this);
> +
> +    ValueOperand lhs = allocator.useValueRegister(masm, reader.valOperandId());
> +    ValueOperand rhs = allocator.useValueRegister(masm, reader.valOperandId());
> +    bool lhsIsString = reader.readBool();

Nit: we should remove this and just check lhs.isString() in the VM function.

Comment 41

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

Attachment: [Part 18] Add StringObject Concat

Found the issue: Was the PopValues(2) that had been imported from the SharedIC version.

Not clear why they're not needed in CacheIR, but removing them i've got clean builds [1]

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f724e070b2bd9fc14f1d4338696a737e081933d6

Comment 42

[Ted Campbell [:tcampbell]]

PopValues(2) was to clear: https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/js/src/jit/SharedIC.cpp#1039-1040

Let me know if you want to talk about the expression decompiler that comment refers to.

Comment 43

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

(In reply to Ted Campbell [:tcampbell] from comment #42)
> PopValues(2) was to clear: <snip> 

It was staring me right in the face :P Thanks. I would love to chat about the expression decompiler another time. 

I've circled back to trying to figure out what to do with the Baseline Inspector. It's a little awkward, because a lot of the checks in the baseline inspector are based on notions of IC Stub kinds that fall away with CacheIR. 

I've been thinking about how to provide the baseline inspector with information. 

1. Create a new enum of stub kinds (essentially resuccitating the 
   old SharedKinds like "BinaryDouble"), from which the inspector
   could infer the MIR type, and store it on the CacheIRStubInfo. 

2. Attempt to parse all the CacheIR bytecode, to ensure we get
   the correct return type. The most awkward piece of this is 
   that ultimately we'd have to recapitulate a bunch of the logic
   in CacheIR.cpp to successfully parse all the bytecodes. See 
   BaselineInspector::instanceOfData [1]. This totally makes sense 
   in some circumstances, so I'm not proposing walking away from it 
   entirely, but in circumstances where there's a large range of expected
   CacheIR bytecode it can be awkward. 

The third option I like, but is a little philosophically divergent and so I wanted to run it past you: 

3. Create a NOP bytecode for metadata , and use it to directly 
   encode the information that the baseline inspector is interested
   in. 

There's a couple options in the metadata bytecode design space (i.e. one op with multiple arguments, each argument consisting of a bit of information the inspector could want, or multiple bytecodes each with ~1 argument). 

For the specific case of BaselineInspector::expectedResultType [2] we could add a metadata op like "ExpectedResult" which stores a MIRType. 


[1]: https://searchfox.org/mozilla-central/source/js/src/jit/BaselineInspector.cpp#1385-1434
[2]: https://searchfox.org/mozilla-central/source/js/src/jit/BaselineInspector.cpp#322-350

Comment 44

[Jan de Mooij [:jandem]]

If we write BaselineInspecter::expectedResultType like this: skip any LHS/RHS type guards, then a switch-statement for all int32/double add/sub/.. CacheIR instructions, does that really get unwieldy?

I didn't like recording metadata for BaselineInspector for the property access ICs, because it would make that code a lot more complicated, but it could make sense to have a nop instruction here. But I think I'd still prefer parsing the IR if it doesn't get too awkward.

Comment 45

[Jan de Mooij [:jandem]]

Comment on attachment 8975048 [details] [diff] [review]
[Part 1] Implement a subset of JSOP_ADD in CacheIR

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

Very exciting to see this happen! Some questions below.

::: js/src/jit/CacheIR.h
@@ +1785,5 @@
> +  public:
> +    BinaryArithIRGenerator(JSContext* cx, HandleScript, jsbytecode* pc, ICState::Mode,
> +                           JSOp op, HandleValue lhs, HandleValue rhs, HandleValue res);
> +
> +    bool tryAttachStub();

The patch is missing the definition of this method I think.

::: js/src/jit/CacheIRCompiler.cpp
@@ +689,5 @@
>  }
>  
> +
> +/**
> + * This address should be consumed as soon as posible, as an intervening 

Nit: trailing whitespace and once below

I don't think this function should pop/free the Value on the stack without updating the OperandLocation - can't we just use the operand's Address without freeing/popping it?

::: js/src/jit/CacheIRCompiler.h
@@ +321,4 @@
>  
>      void popPayload(MacroAssembler& masm, OperandLocation* loc, Register dest);
>      void popValue(MacroAssembler& masm, OperandLocation* loc, ValueOperand dest);
> +    Address  popValueAddress(MacroAssembler& masm, OperandLocation* loc);

Nit: double space after Address

::: js/src/jit/IonBuilder.cpp
@@ +3539,4 @@
>  
>  
>      MInstruction* stub = nullptr;
> +    static const char* disable_cacheir_binary_stubs = getenv("DISABLE_CACHEIR_BINARY");

We shouldn't call getenv without some sort of caching (and in the Baseline code), but if you want this flag we should just add a JitOption for it (you can also set these with environment variables, something like JIT_OPTION_yourOption=1).

@@ +3551,5 @@
>        case JSOP_ADD:
> +        // If not disabled, prefer the cache IR stub.
> +        if (!disable_cacheir_binary_stubs) {
> +            stub = MBinaryCache::New(alloc(), left, right);
> +	        break;

Nit: indentation of this line is wrong

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +557,5 @@
>          allocator.initInputLocation(0, ic->input());
>          break;
>        }
> +      case CacheKind::BinaryArith: {
> +        IonBinaryArithIC *ic = ic_->asBinaryArithIC();

Nit: * goes with the type

@@ +562,5 @@
> +        ValueOperand output = ic->output();
> +
> +        available.add(output);
> +
> +        liveRegs_.emplace(ic->liveRegs());

Maybe here we can assert liveRegs_ does not contain FloatReg0 and FloatReg1?

Comment 46

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

Attachment: [Part 19] Teach the Baseline inspector about CacheIR stubs for BinaryArith

Ok, it turns out you can parse the IR with relative ease :D

Comment 47

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

Attachment: [Part 20] Remove Shared IC support

Comment 48

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

Attachment: [Part 1] Implement a subset of JSOP_ADD in CacheIR

Definitely assembled the patches slightly incorrectly.

Here's an update that has the bits, with nits addressed (including popValueAddress)

Comment 49

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

Attachment: [Part 2] Implement a subset of JSOP_SUB in CacheIR

Comment 50

[Jan de Mooij [:jandem]]

Comment on attachment 8976353 [details] [diff] [review]
[Part 1] Implement a subset of JSOP_ADD in CacheIR

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

Fantastic, thanks.

::: js/src/jit/CacheIR.cpp
@@ +4996,5 @@
> +
> +    if (!lhs_.isInt32() || !rhs_.isInt32())
> +        return false;
> +
> +

Uber nit: remove one of the blank lines

::: js/src/jit/IonBuilder.cpp
@@ +3550,5 @@
>        case JSOP_ADD:
> +        // If not disabled, prefer the cache IR stub.
> +        if (!JitOptions.disableCacheIRBinaryArith) {
> +            stub = MBinaryCache::New(alloc(), left, right);
> +	        break;

Nit: fix indentation

::: js/src/jit/SharedIC.cpp
@@ +778,5 @@
>          return true;
>  
> +    // Try to use a CacheIR stub first. If that succeeds, then we're done. Otherwise, we
> +    // need to try to attach a shared stub.
> +    if (engine == ICStubCompiler::Engine::Baseline && payload && !JitOptions.disableCacheIRBinaryArith) {

Nit: payload/frame should always be non-null so we could remove the payload && part.

::: js/src/shell/js.cpp
@@ +8462,5 @@
>              jit::JitOptions.disableCacheIR = false;
>          else if (strcmp(str, "off") == 0)
>              jit::JitOptions.disableCacheIR = true;
> +        else if (strcmp(str, "nobinary") == 0)
> +            jit::JitOptions.disableCacheIRBinaryArith = true;

Clever

Comment 51

[Jan de Mooij [:jandem]]

Comment on attachment 8975051 [details] [diff] [review]
[Part 4] Implement CacheIR IC for bitwise operations on Int32s

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

::: js/src/jit/CacheIR.cpp
@@ +5003,1 @@
>          return false;

(I'd say add {} here, with { on its own line, because multi-line condition, but I assume this is temporary so it's fine to keep as-is.)

@@ +5044,2 @@
>  bool
>  BinaryArithIRGenerator::tryAttachBooleanWithInt32()

Maybe we can merge tryAttachInt32 and tryAttachBooleanWithInt32 at some point because they're so similar.

Comment 52

[Jan de Mooij [:jandem]]

Comment on attachment 8975055 [details] [diff] [review]
[Part 6] Allow allocateFixedRegister to spill in order to acquire a fixed register

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

Nice find.

::: js/src/jit/CacheIRCompiler.cpp
@@ +411,4 @@
>          return;
>      }
>  
> +    // Register may be avaiable only after spilling contents.

Nit: available

Comment 53

[Jan de Mooij [:jandem]]

Comment on attachment 8975058 [details] [diff] [review]
[Part 9] Implement flexible{quotient,remainder}32

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

::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +604,5 @@
> +    extern MOZ_EXPORT int64_t __aeabi_idivmod(int,int);
> +}
> +
> +inline void
> +EmitRemainderOrQuotient(bool isRemainder, MacroAssembler& masm,

Nit: maybe move to the cpp file because it's quite a lot of code

@@ +611,5 @@
> +    // Currently this helper can't handle this situation.
> +    MOZ_ASSERT(lhsOutput != rhs);
> +
> +    if (HasIDIV()) {
> +        if (isRemainder) {

Nit: no {}

@@ +617,5 @@
> +        } else {
> +            masm.quotient32(rhs, lhsOutput, isSigned);
> +        }
> +    } else {
> +        masm.setupAlignedABICall();

I think we have to save/restore volatile registers here. In Ion, LSoftDivI is marked as a call instruction so there regalloc does it for us. The caller could do:

  LiveRegisterSet volatileRegs(GeneralRegisterSet::Volatile(), liveVolatileFloatRegs());

And then this function could take a |const LiveRegisterSet& volatileRegs| argument. Then we can masm.PushRegsInMask(volatileRegs) and PopRegsInMask here (just have to be careful not to clobber your result register with this..)

@@ +622,5 @@
> +        masm.passABIArg(rhs);
> +        masm.passABIArg(lhsOutput);
> +        masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, __aeabi_idivmod), MoveOp::GENERAL,
> +                         CheckUnsafeCallWithABI::DontCheckOther);
> +        if (isRemainder) {

Nit: no {}

::: js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
@@ +281,5 @@
>      mov(edx, eax);
>  }
>  
> +inline void
> +EmitRemainderOrQuotient(bool isRemainder, MacroAssembler& masm,

Same here regarding moving to the cpp file.

@@ +291,5 @@
> +    // For quotient32 and remainder32
> +    // On x86_shared, srcDest must be eax and edx will be clobbered.
> +    //
> +    Register regForScratch = edx;
> +    Register regForDiv = eax;

Shouldn't this be edx if we're interested in the remainder?

@@ +331,5 @@
> +        masm.mov(regForDiv, lhsOutput);
> +        masm.pop(regForDiv);
> +    }
> +
> +    if (rhs != regForRhs && regForRhs != lhsOutput) {

Nit: no {}

Comment 54

[Jan de Mooij [:jandem]]

Comment on attachment 8975059 [details] [diff] [review]
[Part 10] Implement JSOP_DIV in CacheIR

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +2139,5 @@
> +
> +    // Reload lhs, and recompute quotient for return.
> +    masm.pop(lhs);
> +
> +    masm.flexibleQuotient32(rhs, lhs, false);

Because IDIV is so slow, we should try to avoid doing two of them... Do you have perf numbers for a simple micro-benchmark? Follow-up bug or patch would be good if it's very measurable.

Comment 55

[Jan de Mooij [:jandem]]

Comment on attachment 8975066 [details] [diff] [review]
[Part 17] Add String+String Concatenation to CacheIR

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

Nice! r=me with comments below addressed. Sorry for the slow reviews :/

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +2480,5 @@
> +    masm.push(rhs);
> +    masm.push(lhs);
> +
> +    //MG:XXX: Unlike the shared IC stubs, I was unable to get
> +    //        tailCallVM working here.

Non-tail call should be fine; I think we can remove the comment.

::: js/src/jit/CacheIR.cpp
@@ +4957,4 @@
>          return true;
>      if (tryAttachBooleanWithInt32())
>          return true;
> +    if (tryAttachStringConcat()) 

Nit: trailing whitespace

@@ +5117,5 @@
>      return true;
>  }
>  
> +bool 
> +BinaryArithIRGenerator::tryAttachStringConcat()

Same here.

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +2586,5 @@
>  }
> +
> +
> +static bool
> +DoConcatStrings(JSContext* cx, HandleString lhs, HandleString rhs, MutableHandleValue res)

We should move this function to jit/VMFunctions.cpp/h to avoid duplicating it here and in the Baseline code.

Actually, I think we can call ConcatStrings<CanGC> directly and then do this to box the JSString*:

  masm.tagValue(JSVAL_TYPE_STRING, ReturnReg, output.valueReg());

That assumes we always have a Value output register for this IC but I think that's true. Same for the Baseline code - this was not possible with tail calls but now it is :)

Comment 56

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

Attachment: [Part 10] Implement flexibleDivMod

Comment 57

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

Attachment: [Part 11] Implement JSOP_DIV in CacheIR

Comment 58

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

Attachment: [Part 21] Reorder attachment order to get correct fallthrough behaviour

This ensures we get the best stub for each case that is most likely to cover future attachments

Comment 59

[Jan de Mooij [:jandem]]

Comment on attachment 8983082 [details] [diff] [review]
[Part 11] Implement JSOP_DIV in CacheIR

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

Great, thanks.

Comment 60

[Jan de Mooij [:jandem]]

Comment on attachment 8975891 [details] [diff] [review]
[Part 19] Teach the Baseline inspector about CacheIR stubs for BinaryArith

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

r=me with nits addressed.

::: js/src/jit/BaselineInspector.cpp
@@ +321,5 @@
>  
> +// Process the type guards in the stub in order to reveal the
> +// underlying operation.
> +void
> +SkipBinaryGuards(CacheIRReader& reader) {

Nit: { on its own line

@@ +324,5 @@
> +void
> +SkipBinaryGuards(CacheIRReader& reader) {
> +    bool found = true;
> +    do {
> +        ObjOperandId objId;

Nit: rm unused variable.

@@ +345,5 @@
> +        {
> +            reader.skip(); // Skip over operandId
> +            continue;
> +        }
> +        found = false;

Maybe it's simpler to remove the |found| bool, make this a |while (true) {| loop, and just |return;| here.

@@ +350,5 @@
> +    } while (found);
> +
> +}
> +
> +MIRType

Nit: s/MIRType/static MIRType/ - SkipBinaryGuards can also be static.

@@ +353,5 @@
> +
> +MIRType
> +ParseCacheIRStub(ICStub* stub)
> +{
> +    ICCacheIR_Regular* cacheir_stub = stub->toCacheIR_Regular();

Nit: cacheirStub

Comment 61

[Jan de Mooij [:jandem]]

Comment on attachment 8983081 [details] [diff] [review]
[Part 10] Implement flexibleDivMod

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

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +5982,5 @@
> +
> +    if (HasIDIV()) {
> +        mov(lhsOutput, remainder);
> +        remainder32(rhs, remainder, isUnsigned);
> +        quotient32(rhs, lhsOutput, isUnsigned);

Nit: file a followup bug to add an enum class argument so we can say which one (div or mod) we want? Then we can micro-optimize on ARM.

@@ +5988,5 @@
> +        PushRegsInMask(volatileRegs);
> +        setupAlignedABICall();
> +        passABIArg(rhs);
> +        passABIArg(lhsOutput);
> +        callWithABI(JS_FUNC_TO_DATA_PTR(void*, __aeabi_idivmod), MoveOp::GENERAL,

Should this be uidivmod if isUnsigned? If isUnsigned is always false, we could also remove the argument. Either way is fine with me.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp
@@ +404,5 @@
> +        masm.mov(eax, lhsOutput);
> +        masm.pop(eax);
> +    }
> +
> +    if (rhs != regForRhs && regForRhs != lhsOutput && regForRhs != remainderOutput) {

Nit: no {}

Comment 62

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

(In reply to Jan de Mooij [:jandem] from comment #61)
> Comment on attachment 8983081 [details] [diff] [review]
> [Part 10] Implement flexibleDivMod
>
> Nit: file a followup bug to add an enum class argument so we can say which
> one (div or mod) we want? Then we can micro-optimize on ARM.

Currently we only use flexibleDivMod where we actually want both, and have a separate implementation for flexibleQuotient and flexibleRemainder where we don't need both. However, I plan on opening a followup item to try to fold the underlying implementations together where possible.

Comment 63

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

Attachment: Rollup of 21 patches for fuzzing, apply to m-c 0b5495dc100

Attached patch for fuzzing 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da995d96b05959de5c6b8abe6906b5b8503b0527 is the try build that says to me it's likely good to fuzz.

Comment 64

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

Comment on attachment 8985681 [details]
Rollup of 21 patches for fuzzing, apply to m-c 0b5495dc100

Setting feedback? flag for fuzzing of this patch.

Comment 65

[Ted Campbell [:tcampbell]]

Comment on attachment 8975054 [details] [diff] [review]
[Part 5] Implement branchMul32 in MacroAssembler

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

I think there are missing test runs or test cases for the ARM64 mul overflow case. Code looks to be missing a branch.

Comment 66

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

Attachment: [Part 5] Implement branchMul32 in MacroAssembler

Regarding Comment 65: The branch was there, but hidden, and it wasn't clear 
that it would only work for Assembler::Overflow. So I have cleaned up this patch.

Comment 67

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

Attachment: [Part 10] Implement flexibleDivMod

Speaking of ARM64 -- your poking had me find a mistake in the ARM64 
implementation of flexibleDivMod that Jan had already reviewed. 

Would you mind taking a second look at the arm64 implementation to make 
sure it makes sense to you. I needed an extra register to avoid clobbering
the div result (ultimately only the remainder register was valid in the
previous version of the patch)

Comment 68

[Ted Campbell [:tcampbell]]

Comment on attachment 8975057 [details] [diff] [review]
[Part 8] Implement JSOP_MUL in CacheIR

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

I'd like to see a reduced version of CacheIRCompiler::emitInt32MulResult if it makes sense

::: js/src/jit/CacheIRCompiler.cpp
@@ +169,4 @@
>  }
>  
>  Register
> +CacheRegisterAllocator::useFixedRegister(MacroAssembler& masm, TypedOperandId typedId, Register reg)

This seems dead in this patch

@@ +2076,5 @@
>      return true;
>  }
> +
> +bool
> +CacheIRCompiler::emitInt32MulResult()

In the |-0| path, lhs should still be holding 0 and we can hopefully avoid the Imm32(0). Once we do that, the EmitStoreResult can be shared.

Comment 69

[Ted Campbell [:tcampbell]]

Comment on attachment 8975056 [details] [diff] [review]
[Part 7] Clarify restrictions on imul and remove un-needed restriction

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

Once we land this, we should ask our WASM folks to revisit this. They seem to set up for EAX/EDX but then use the two-operand form of imull anyways

Comment 70

[Ted Campbell [:tcampbell]]

Comment on attachment 8975062 [details] [diff] [review]
[Part 13] Support Double DIV and Double MOD

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +2078,5 @@
> +
> +    masm.setupUnalignedABICall(scratch);
> +    masm.passABIArg(FloatReg0, MoveOp::DOUBLE);
> +    masm.passABIArg(FloatReg1, MoveOp::DOUBLE);
> +    masm.callWithABI(BitwiseCast<void*, double (*)(double, double)>(js::NumberMod),

Use JS_FUNC_TO_DATA_PTR for consistency.

Comment 71

[Ted Campbell [:tcampbell]]

Comment on attachment 8975064 [details] [diff] [review]
[Part 15] Support shifts in CacheIR

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +2277,5 @@
> +    Register rhs = allocator.useRegister(masm, reader.int32OperandId());
> +
> +
> +    //Mask shift amount as specified by 12.9.3.1 Step 7
> +    masm.and32(Imm32(0x1F), rhs);

I wonder if we want to refactor the shift APIs to absorb this check since JS and WASM both spec this behavior and we don't need the masking on X86/AMD64

@@ +2327,5 @@
> +        masm.convertUInt32ToDouble(lhs, ScratchDoubleReg);
> +        masm.boxDouble(ScratchDoubleReg, output.valueReg(), ScratchDoubleReg);
> +    } else {
> +        masm.branchTest32(Assembler::Signed, lhs, lhs, failure->label());
> +        EmitStoreResult(masm, lhs, JSVAL_TYPE_INT32, output);

This EmitStoreResult could be shared at the end to reduce code size.

Comment 72

[Ted Campbell [:tcampbell]]

Comment on attachment 8975063 [details] [diff] [review]
[Part 14] Implement flexible shift macroassembler helpers

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

As mentioned in other comment, I'd like to consider (in a follow-up) integrating the 5-bit masking into API since it is an easy machine optimization and our input languages spec this behavior.

Comment 73

[Ted Campbell [:tcampbell]]

Comment on attachment 8986054 [details] [diff] [review]
[Part 5] Implement branchMul32 in MacroAssembler

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

Thanks for clarifying the condition stuff.

::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +1312,5 @@
> +void
> +MacroAssembler::branchMul32(Condition cond, T src, Register dest, Label* label)
> +{
> +    vixl::UseScratchRegisterScope temps(this);
> +    MOZ_ASSERT(cond == Assembler::Overflow);

For my sanity, move the assert up a line to match the ARM variant.

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +642,5 @@
> +template <typename T>
> +void
> +MacroAssembler::branchMul32(Condition cond, T src, Register dest, Label* overflow)
> +{
> +    switch (cond) {

I'd probably prefer this to be an assert like the ARM variants.

Comment 74

[Ted Campbell [:tcampbell]]

Comment on attachment 8975589 [details] [diff] [review]
[Part 18] Add StringObject Concat

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

Clearing review until expression decompiler stuff is addressed. Also those XXX comments can be removed.

Comment 75

[Ted Campbell [:tcampbell]]

Comment on attachment 8975065 [details] [diff] [review]
[Part 16] Expand IC coverage by reducing guards to attach ICs where appropriate

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

This is partially reverted by Part 21, so I'd prefer we just drop this patch and merge into Part 21.

Comment 76

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

Attachment: Expression decompilation test case

The attached test case reveals some issues with the expression decompiler and the String+Object IC: 

Without CacheIR

    $ ./dist/bin/js  --cache-ir-stubs=nobinary ../jit-test/tests/cacheir/expression_decompile_strobj.js
    TypeError: can't convert b to primitive type: its [Symbol.toPrimitive] property is not a function
    < snip repeats> 

With CacheIR

    $ ./dist/bin/js   ../jit-test/tests/cacheir/expression_decompile_strobj.js
    TypeError: can't convert b to primitive type: its [Symbol.toPrimitive] property is not a function
    TypeError: can't convert b to primitive type: its [Symbol.toPrimitive] property is not a function
    TypeError: can't convert b to primitive type: its [Symbol.toPrimitive] property is not a function
    TypeError: can't convert b to primitive type: its [Symbol.toPrimitive] property is not a function
    TypeError: can't convert ({[Symbol.toPrimitive]:3}) to primitive type: its [Symbol.toPrimitive] property is not a function
    <snip repeats>

I'll see if I can't figure out how to fix this. (My earliest attempt marking the helper as "PopValues(2)", and adding the extra pushes was thwarted by an assertion in IonCacheIRCompiler about the frame size.

Comment 77

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

Comment 76 seems to point out a hard problem for CacheIR: 

It seems however that because CacheIR stubs are using the stack differently (composition of opcodes, combined with the CacheIR register allocator means I don't have total control of the stack layout at the point where in theory I would want to 'sync the stack the expression decompiler'. 

It's worth noting: The output that the ICs produce is the same as is produced under --enable-more-deterministic under the interpreter.

Comment 78

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

Attachment: [Part 8] Implement JSOP_MUL in CacheIR

This requires building usedFixedRegister to handle the x86 register requirments
of multiply operations.

Comment 79

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

Try build up to part 8: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75cb3784d79258fa8a15b78ef991f4a480b0e189

Comment 80

[Ted Campbell [:tcampbell]]

Comment on attachment 8986507 [details] [diff] [review]
[Part 8] Implement JSOP_MUL in CacheIR

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

Thanks. Fix commit message too

::: js/src/jit/CacheIRCompiler.cpp
@@ +2056,5 @@
> +    Label maybeNegZero, done;
> +    masm.mov(lhs, scratch);
> +    masm.branchMul32(Assembler::Overflow, rhs, lhs, failure->label());
> +    masm.branchTest32(Assembler::Zero, lhs, lhs, &maybeNegZero);
> +    masm.jump(&done);

s/done/storeResult/

Comment 81

[Ted Campbell [:tcampbell]]

Comment on attachment 8986055 [details] [diff] [review]
[Part 10] Implement flexibleDivMod

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

::: js/src/jit/arm64/MacroAssembler-arm64.cpp
@@ +1885,5 @@
> +    if (isUnsigned)
> +        Udiv(ARMRegister(srcDest, 32), ARMRegister(srcDest, 32), ARMRegister(rhs, 32));
> +    else
> +        Sdiv(ARMRegister(srcDest, 32), ARMRegister(srcDest, 32), ARMRegister(rhs, 32));
> +    Mul(scratch, scratch, ARMRegister(rhs, 32));

|scratch| looks used uninitialized. We should figure out why tests still pass =\

Comment 82

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

Attachment: [Part 10] Implement flexibleDivMod

Corrected Arm64 implementation of flexibleDivMod. As to why we didn't see 
tests fail, it was unfortunately the side effect of inline caches: The invalid 
results tripped the stub-failure condition, which silently meant correct output
despite the incorrect computation.

Comment 83

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

Attachment: [Part 16] Support shifts in CacheIR

The previous version of this patch had an error only revealed on 
arm64 relating to the results of the double-returning version of 
USRH. This corrects the control flow.

Comment 84

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

Attachment: [Part 18] Add StringObject Concat

This version of the patch has support for the expression decompiler added 
for baseline. Talking with Jan on IRC, and the conclusion seems to be doing 
the same for Ion would be more work than worthwhile. 

(We would have to special case the BinaryArith IC dispatch code to be more like
emitSharedStub at the bare minimum)

Comment 85

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

Attachment: [Part 20] Reorder attachment order to get correct fallthrough behaviour

This ensures we get the best stub for each case that is most likely to cover future attachments

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc6349409e14
[Part 0] Add test case for binary arithmetic operations r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/e45d06e5a967
[Part 1] Implement a subset of JSOP_ADD in CacheIR r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/be222e6b343f
[Part 2] Implement a subset of JSOP_SUB in CacheIR r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8d18214f53
[Part 3] Implement ADD+SUB for Boolean + Double|Int32 r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9452e41ec38
[Part 4] Implement CacheIR IC for bitwise operations on Int32s r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/abf6c165d869
[Part 5] Implement branchMul32 in MacroAssembler r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/da261f1d340e
[Part 6] Allow allocateFixedRegister to spill in order to acquire a fixed register r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/28ae7dd30047
[Part 7] Clarify restrictions on imul and remove un-needed restriction r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/948fcbfaa6f3
[Part 8] Implement JSOP_MUL in CacheIR r=tcampbell

Comment 87

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

Backed out for failing xpcshell's test_cssColor-02.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca88f0e401147602351a71f043038432453218d0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=948fcbfaa6f32fd8d92e7137030b6238430c4711&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=184970943&repo=mozilla-inbound

16:57:24     INFO -  TEST-START | devtools/client/shared/test/unit/test_cssColor-02.js
16:57:24  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/shared/test/unit/test_cssColor-02.js | xpcshell return code: 0
16:57:24     INFO -  TEST-INFO took 299ms
16:57:24     INFO -  >>>>>>>
16:57:24     INFO -  PID 11524 | JavaScript strict warning: resource://devtools/shared/Loader.jsm, line 224: ReferenceError: reference to undefined property "name"
16:57:24     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
16:57:24     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_cssColor-02.js | run_test/< - [run_test/< : 24] aliceblue was able to cycle back to the original value - true == true
16:57:24     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_cssColor-02.js | run_test/< - [run_test/< : 24] antiquewhite was able to cycle back to the original value - true == true
16:57:24     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_cssColor-02.js | run_test/< - [run_test/< : 24] aqua was able to cycle back to the original value - true == true
16:57:24     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_cssColor-02.js | run_test/< - [run_test/< : 24] aquamarine was able to cycle back to the original value - true == true
16:57:24  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/shared/test/unit/test_cssColor-02.js | run_test/< - [run_test/< : 24] azure was able to cycle back to the original value - false == true
16:57:24     INFO -  Z:/task_1530030712/build/tests/xpcshell/tests/devtools/client/shared/test/unit/test_cssColor-02.js:run_test/<:24
16:57:24     INFO -  Z:/task_1530030712/build/tests/xpcshell/tests/devtools/client/shared/test/unit/test_cssColor-02.js:run_test:17
16:57:24     INFO -  Z:\task_1530030712\build\tests\xpcshell\head.js:_execute_test:520
16:57:24     INFO -  -e:null:1
16:57:24     INFO -  exiting test
16:57:24     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "name"" {file: "resource://devtools/shared/Loader.jsm" line: 224}]"
16:57:24     INFO -  <<<<<<<

Comment 88

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

Attachment: Stop updating operand location in loadDouble

I landed the first eight patches on Tuesday, but they were backed out for
failing xpcshell's test_cssColor-02.js (see Comment 89).

I have a partial view of what's going on here, which is what leads to this 
patch. However, I'm also not entirely sure of how the OperandLocation story 
is supposed to work (hence the f?). 

So, in early versions of this patch set (see Comment 45), we were "moving"
operands into FloatRegisters. As a result, we started setting the
OperandLocation for the code as DoubleReg. Further evolution changed this, and
now the call to ensureDouble just references the location of the input argument,
wherever it may be. 

My understanding is this means I am justified in removing the setDoubleReg calls,
as restoreInputState then needn't do anything on a failure path, as the value
should still be in the right place (modulo potential spilling). 

However, there's a couple pieces I don't understand (and I have still not quite 
nailed down the exact sequence that causes the failure in the cssColor test)

1. If I annotate the restoreInputState double restoration code, and make
   loadDouble infallible, I still see the float register being boxed; this 
   seems crazy (partially that anything works at all), because the only failures
   that can happen in this IC happen during GuardNumber, at which point the 
   value is still in its original location, and so what we'd be boxing would be 
   whatever happened to be in that register (potentially some random value Ion 
   had computed!).

   However, the closer I look at it, the more I don't see how different operand
   locations for a value are kept separate in the failure path code. By this I
   mean, it's not clear if the input restoration code is meant to be 'stateful'?
   By this I mean, it seems that the failure path generation code only looks at
   the 'final' state of the world, but it seems like the state can evolve over
   time, leading to different failure path requirements depending on when the
   failure path is taken. 

   This feels like maybe the root of my misunderstanding. 

2. Are the input operand locations intended to be immutable? The more I think
   about it, the more that makes sense (and the more this seems broken). 

   While it turns out that loadDouble/ emitDoubleAdd is infallible in this
   context (and I plan a followup patch to make sure I take advantage of that), I
   guess I wonder what would happen if we had done some computation using
   FloatReg1, then took a failure path. It seems to me at that point we'd
   'restore' some intermediate value as input to the next IC. 

   This seems to be further justification to not changing the operand location
   to point to the FloatReg.

Comment 89

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

Attachment: Make CacheIRAllocator::loadDouble infallible (Multipart fixup)

For this API fixup, I was thinking before delivery I'd just run it through the 
appropriate patches, without getting the patched patches reviewed. 

Sound good?

Comment 90

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

Comment on attachment 8987607 [details] [diff] [review]
[Part 18] Add StringObject Concat

Clearing review. I forgot to check 32 bit builds! 

Thanks try!

Comment 91

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

Comment on attachment 8985681 [details]
Rollup of 21 patches for fuzzing, apply to m-c 0b5495dc100

Clearing feedback? - I don't think I found things related to this rollup, but there seems to have been enough subsequent revisions to parts of the patch that I'm not prepared to call feedback+ yet.

Comment 92

[Jan de Mooij [:jandem]]

Comment on attachment 8988343 [details] [diff] [review]
Stop updating operand location in loadDouble

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

Yes this makes sense.

Comment 93

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: MIR instruction returned value with unexpected type, at js/src/jit/MacroAssembler.cpp:2019
Build version: mozilla-central revision 0b5495dc100d+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize
Runtime options: --fuzzing-safe --ion-eager --ion-offthread-compile=off

Testcase:

const handler = {}
function testArray(arr) {
    let proxy = new Proxy(arr, handler)
    proxy.sort((x, y) => 1 * x - y);
    arr.sort((x, y) => 1 * x - y);
}
testArray([5, (this), 2, 99]);

Backtrace:

received signal SIGTRAP, Trace/breakpoint trap.
0x000003f09021c843 in ?? ()
#0  0x000003f09021c843 in ?? ()
[...]
#69 0x00007ffff4d004b0 in ?? ()
#70 0x0000000000a8e0b6 in js::ThisThread::GetId () at js/src/threading/posix/Thread.cpp:169
#71 js::CheckThreadLocal::check (this=0x7ffff4d00540) at js/src/threading/ProtectedData.cpp:43
rax	0x80b1	32945
rbx	0x7ffff4d00540	140737300661568
rcx	0x3	3
rdx	0x4	4
rsi	0x7ffff4d00570	140737300661616
rdi	0x3	3
rbp	0x4058c00000000000	4636666922610458624
rsp	0x7fffffffb820	140737488336928
r8	0xfff9800000000000	-1829587348619264
r9	0x4	4
r10	0x1	1
r11	0x7ffff6b977a0	140737332737952
r12	0x4	4
r13	0x3	3
r14	0x4	4
r15	0xfff9800000000000	-1829587348619264
rip	0x3f09021c843	4331745167427
=> 0x3f09021c843:	push   %r10
   0x3f09021c845:	push   %r9

Comment 94

[Jan de Mooij [:jandem]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #88)
> My understanding is this means I am justified in removing the setDoubleReg
> calls,
> as restoreInputState then needn't do anything on a failure path, as the value
> should still be in the right place (modulo potential spilling).

Agreed.

> 1. If I annotate the restoreInputState double restoration code, and make
>    loadDouble infallible, I still see the float register being boxed; this 
>    seems crazy

Yeah it would be good to look into this; IIUC FloatReg0/FloatReg1 are just scratch registers now that are never associated with an input operand and they're marked as temps so the Ion register allocator should never use them for an operand to your IC.

> 2. Are the input operand locations intended to be immutable

Yes.

The only registers a CacheIR instruction is allowed to write to are scratch registers and output registers. It's not okay to modify input registers because Ion code after the IC could depend on the original values.

What happens when we add a failure path is that we record the current locations of each IC input operand and later when we generate the failure path code we compare this to the IC's input state so we can restore it as if nothing happened.

So I think your patch makes sense, good catch. Let me know if things are still unclear.

Comment 95

[Jan de Mooij [:jandem]]

Comment on attachment 8988489 [details] [diff] [review]
Make CacheIRAllocator::loadDouble infallible (Multipart fixup)

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

Suggestion below for follow-up clean up but we can do that after this lands.

::: js/src/jit/CacheIRCompiler.cpp
@@ +127,4 @@
>      }
> +    masm.jump(&done);
> +    masm.bind(&failure);
> +    masm.assumeUnreachable("Missing guard allowed non-number to hit loadDouble");

File a follow-up bug to add ensureDoubleInfallible (or allow a nullptr Label* in ensureDouble)? We could do the assumeUnreachable there if needed. It's a micro-optimization but then we can remove this code and the Labels/jumps here :)

Comment 96

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

Attachment: Rollup2 Apply to m-c 6e8e861540e6

New rollup patch. 

Confirmed the root cause of the bug Christian found in Comment 93 to be the same as the CSS test failure, which is fixed by the patch on Comment 88, included in this Rollup.

Comment 97

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ __aeabi_idivmod]
Build version: mozilla-central revision 6e8e861540e6+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm
Runtime options: --fuzzing-safe --ion-eager --ion-offthread-compile=off --arm-hwcap=vfp min.js

Testcase:

function f(v, e) {
    for (var i = 2; i < 9; i++) v %= e;
}
f(0, 1);

Backtrace:

received signal SIGFPE, Arithmetic exception.
0x085c6577 in __aeabi_idivmod (x=1, y=0) at js/src/jit/arm/Simulator-arm.cpp:53
#0  0x085c6577 in __aeabi_idivmod (x=1, y=0) at js/src/jit/arm/Simulator-arm.cpp:53
#1  0x0860a7dd in js::jit::Simulator::softwareInterrupt (this=0xf6e55000, instr=0xf56966b4) at js/src/jit/arm/Simulator-arm.cpp:2642
#2  0x0860ad16 in js::jit::Simulator::decodeType7 (this=0xf6e55000, instr=0xf56966b4) at js/src/jit/arm/Simulator-arm.cpp:3878
#3  0x0860c612 in js::jit::Simulator::instructionDecode (this=0xf6e55000, instr=0xf56966b4) at js/src/jit/arm/Simulator-arm.cpp:4858
#4  0x0860ccca in js::jit::Simulator::execute<false> (this=0xf6e55000) at js/src/jit/arm/Simulator-arm.cpp:4913
#5  js::jit::Simulator::callInternal (this=0xf6e55000, entry=0x39a4a848 "\360O-\351\004\320M\342\020\212-\355\r\200\240\341h\220\235\345\r\260\240\341t\240\235", <incomplete sequence \345>) at js/src/jit/arm/Simulator-arm.cpp:4993
#6  0x0860cee9 in js::jit::Simulator::call (this=<optimized out>, entry=<optimized out>, argument_count=<optimized out>) at js/src/jit/arm/Simulator-arm.cpp:5076
#7  0x083ef567 in EnterJit (cx=<optimized out>, cx@entry=0xf6e1e800, state=..., code=0x39a5c308 "\004\340-\345\a") at js/src/jit/Jit.cpp:101
#8  0x083f01af in js::jit::MaybeEnterJit (cx=0xf6e1e800, state=...) at js/src/jit/Jit.cpp:163
#9  0x081e9672 in js::RunScript (cx=<optimized out>, state=...) at js/src/vm/Interpreter.cpp:406
#10 0x081e9cc5 in js::InternalCallOrConstruct (cx=<optimized out>, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:493
#11 0x081ea0c0 in InternalCall (cx=cx@entry=0xf6e1e800, args=...) at js/src/vm/Interpreter.cpp:520
#12 0x081ea23f in js::CallFromStack (cx=0xf6e1e800, args=...) at js/src/vm/Interpreter.cpp:526
#13 0x082cb78c in js::jit::DoCallFallback (cx=<optimized out>, frame=0xf5bfff10, stub_=0xf57c5030, argc=2, vp=0xf5bffec0, res=...) at js/src/jit/BaselineIC.cpp:2370
#14 0x0860a510 in js::jit::Simulator::softwareInterrupt (this=0xf6e55000, instr=0xf6e7b164) at js/src/jit/arm/Simulator-arm.cpp:2709
#15 0x0860ad16 in js::jit::Simulator::decodeType7 (this=0xf6e55000, instr=0xf6e7b164) at js/src/jit/arm/Simulator-arm.cpp:3878
#16 0x0860c612 in js::jit::Simulator::instructionDecode (this=0xf6e55000, instr=0xf6e7b164) at js/src/jit/arm/Simulator-arm.cpp:4858
#17 0x0860ccca in js::jit::Simulator::execute<false> (this=0xf6e55000) at js/src/jit/arm/Simulator-arm.cpp:4913
#18 js::jit::Simulator::callInternal (this=0xf6e55000, entry=0x39a4a848 "\360O-\351\004\320M\342\020\212-\355\r\200\240\341h\220\235\345\r\260\240\341t\240\235", <incomplete sequence \345>) at js/src/jit/arm/Simulator-arm.cpp:4993
#19 0x0860cee9 in js::jit::Simulator::call (this=<optimized out>, entry=<optimized out>, argument_count=<optimized out>) at js/src/jit/arm/Simulator-arm.cpp:5076
#20 0x083ef567 in EnterJit (cx=<optimized out>, cx@entry=0xf6e1e800, state=..., code=0x39a5a858 "\004\340-\345\a") at js/src/jit/Jit.cpp:101
#21 0x083f01af in js::jit::MaybeEnterJit (cx=0xf6e1e800, state=...) at js/src/jit/Jit.cpp:163
[...]
#31 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9379
eax	0x1	1
ebx	0x39a5d308	967168776
ecx	0x1	1
edx	0x0	0
esi	0xf6e55000	-152743936
edi	0x85c6570	140273008
ebp	0xffffc1a8	4294951336
esp	0xffffc1a8	4294951336
eip	0x85c6577 <__aeabi_idivmod(int, int)+7>
=> 0x85c6577 <__aeabi_idivmod(int, int)+7>:	idivl  0xc(%ebp)
   0x85c657a <__aeabi_idivmod(int, int)+10>:	pop    %ebp


This is a frequent issue, I had to disable further patch testing in ARM.

Comment 98

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

Attachment: Fixup: Correct argument order for calls to {u,}idivmod on ARM32

In Comment 97 :decoder pointed out a test flag I had not been using '--arm-hwcap=vfp'. 

It turns out that this was the key to taking the fallback callout path on ARM, where
I had actually provided the arguments backwards (hence the failure on the test case in
Comment 97). 

Many thanks to :decoder for the find. 

With this fix, I now pass my test cases running

    ../jit-test/jit_test.py --args='--arm-hwcap=vfp' --jitflags=all ./dist/bin/js

Comment 99

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

let count = 0;
function testMathyFunction(f, inputs) {
    for (var j = 0; j < inputs.length; ++j)
        for (var k = 0; k < inputs.length; ++k) {
            let a = f(inputs[j], inputs[k]);
            count += 1;
            if (a) {
                print("Number " + count + ": " + a);
            }
        }
}
    mathy0 = function(x, y) {
        return (x / (y ? x : -Number.MAX_SAFE_INTEGER) > Math.fround(+y & +0x100000000) ** Math.fround(y))
    }
    testMathyFunction(mathy0, /*MARR*/ [
        [1], , , [1],
        [1], , [1], , , [1], , [1], , , [1],
        [1], , [1], , , [1],
        [1], , [1], , [1],
        [1],
        [1], , -1 / 0
    ])
testMathyFunction(mathy0, [, -Number.MAX_VALUE, Number.MIN_SAFE_INTEGER, 0x100000001, 0x07fffffff, -0x07fffffff, 0 / 0, Number.MIN_VALUE, -0x0ffffffff, Number.MAX_SAFE_INTEGER, 0x0ffffffff, -0x100000000, , 1 / 0, 0x080000000, -1 / 0, 0x100000000])


$ ./js-64-dm-linux-b119bfe12f43 --fuzzing-safe --no-threads testcase.js | grep 1002
$

$ ./js-64-dm-linux-b119bfe12f43 --fuzzing-safe --no-threads --ion-eager testcase.js | grep 1002
Number 1002: true
$

(rev b119bfe12f43 came from applying the patch)

For reference:

$ ./js-64-dm-linux-6e8e861540e6 --fuzzing-safe --no-threads testcase.js | grep 1002
Number 1002: true
$

$ ./js-64-dm-linux-6e8e861540e6 --fuzzing-safe --no-threads --ion-eager testcase.js | grep 1002
Number 1002: true
$

Comment 100

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

Comment 99 came from a shell compiled with --enable-more-deterministic

Comment 101

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

Attachment: Fixup: Don't accidentally clobber remainder output

The fix for the lovely test case provided by :gkw in Comment 99.

The complexity of this code is quite a bit higher than I'd like. 

I really would love it if I could write a unit test for flexibleDivMod.

Comment 102

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

Discovered testJitMacroAssembler.cpp; will take a stab at unit testing tomorrow.

Comment 103

[Ted Campbell [:tcampbell]]

Comment on attachment 8990137 [details] [diff] [review]
Fixup: Don't accidentally clobber remainder output

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

Nice detective work!

Fix the one comment. And consider using more named temporaries, especially when matching pushes and pops with complex conditions. If the rest of the function is more of the same we can deal with later.

(Please mess with your diff settings to include more context lines. mach bootstrap will offer to do this for you if you run it)

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp
@@ +381,4 @@
>      // If lhs isn't already in eax, preserve eax, and
>      // move lhs into it.
>      if (lhsOutput != eax) {
> +        // don't clobber remainder output

// If eax is the remainder output, we don't need to preserve old value

@@ +398,5 @@
>  
>      // Quotient is in eax and remainder is in edx right now
> +    // If remainderOutput is eax, defer unloading until after
> +    // we have unloaded lhsOutput
> +    if (remainderOutput != edx && remainderOutput != eax)

Too much spaghetti! At least use a temporary variable to track this deferment instead of copying the slightly altered condition below.

@@ +405,5 @@
>      // Store result into output reg, and restore eax,
>      // unless lhsOutput is already eax.
>      if (lhsOutput != eax) {
>          masm.mov(eax, lhsOutput);
> +        if (remainderOutput != eax)

Maybe also add a preserveEax flag for this condition too so it is easy to match the push and pop.

Comment 104

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

Attachment: [Part 10] Implement flexibleDivMod

A new reworked implementation of flexibleDivMod for x86 that uses 
the MoveResolver to deal with potential clobbers. 

IMO, a huge step forward. Should avoid all potential issues like 
the one Gary found in 99. 

Includes unit test that does 9/4 == (2,1) across the crossproduct
of all distinct register sets. (The unit test triggered the rewrite
when I discovered yet-another-unhandled case in the spaghetti).

(Includes a debug printing setup that uses <vector> and <string>. 
 not strictly necessary, and can be pulled; just produces nicer 
 output with very little hassle)

Comment 105

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

Attachment: [Part 18] Add StringObject Concat

Whoops. Forgot about getting this one back up.

Comment 106

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

Attachment: [Fixup Part 10] Ensure all registers get saved in testJitMacroAsssembler

This fix is required because the new unit tests will clobber registers, and so we
should save them. We need to save all registers as the other tests rely on the
volatile registers being saved (I tried saving just the non-volatile registers,
as you might expect the requirement to be, but other tests started failing).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0312c0b7baebdb45584b2049f27c119d4d2481d7

Comment 107

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

Attachment: Bug1438727.Rollup3.ApplyTo.6e8e861540e6.patch

Comment 108

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

Attachment: Bug1438727.Rollup3.ApplyTo.5a5c74e9ccc0.patch

Rebased to m-c as requested! Thanks again!

Comment 109

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

Attachment: [Part 22] Teach Baseline Inspector to infer binary arith cache types

Found that I had missed this in the baseline inspector when doing my removal patch up 
again.

Comment 110

[Ted Campbell [:tcampbell]]

Comment on attachment 8991143 [details] [diff] [review]
[Fixup Part 10] Ensure all registers get saved in testJitMacroAsssembler

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

The original need to preserve volatile regs seems weird to me, but your version seems like the most sensible overall here.

Comment 111

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

Attachment: [Part 9 Fixup] Unit Test flexibleRemainder32 and flexibleQuotient32

Might as well include these unit tests I also wrote.

Comment 112

[Ted Campbell [:tcampbell]]

Comment on attachment 8991376 [details] [diff] [review]
[Part 9 Fixup] Unit Test flexibleRemainder32 and flexibleQuotient32

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

Testing the various register combinations was nice :)

::: js/src/jsapi-tests/testJitMacroAssembler.cpp
@@ +119,5 @@
> +    StackMacroAssembler masm(cx);
> +
> +    if (!Prepare(masm))
> +        return false;
> +    fprintf(stderr, "Generating flexibleRemainder\n");

drop the printfs

@@ +127,5 @@
> +
> +    AllocatableGeneralRegisterSet leftOutputHandSides(GeneralRegisterSet::All());
> +
> +    // Preserve Frame Pointer
> +    masm.push(FramePointer);

If this isn't needed, we should remove to avoid misleading others.

Comment 113

[Jan de Mooij [:jandem]]

Comment on attachment 8991365 [details] [diff] [review]
[Part 22] Teach Baseline Inspector to infer binary arith cache types

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

I think we could/should look at the arithmetic instruction type instead of looking at the *operand* types? I remember reviewing a patch to skip type guard instructions in BaselineInspector so maybe we could use that here?

::: js/src/jit/BaselineInspector.cpp
@@ +490,5 @@
> +// Returns false if the reader will not be in a correct state
> +// to do subsequent reads and get a CacheOp.
> +static bool
> +UpdateGuardInfo(CacheIRReader& reader, const CacheOp op,
> +                bool& sawInt32, bool& sawDouble, bool& sawOther)

Nit: we use pointers for out-params to make it more obvious at the call site that we're modifying them.

@@ +496,5 @@
> +    int skips = 0;
> +    switch (op) {
> +        case CacheOp::GuardIsInt32:
> +        case CacheOp::GuardIsBoolean:
> +            sawInt32 = true;

Nit: 2 space indents for case/default labels and the code after them.

@@ +554,5 @@
>              sawDouble = true;
>              break;
> +          case ICStub::CacheIR_Regular:
> +            if (!CheckBinaryArithStubArgs(stubs[i]->toCacheIR_Regular(),
> +                                         sawInt32, sawDouble, sawOther))

Nit: fix the indentation here (one more space?)

@@ +572,5 @@
>          return true;
>      }
>  
>      MOZ_ASSERT(sawInt32);
> +    if (sawInt32)

Why this if-statement if the previous line asserts it's true?

Comment 114

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ js::jit::Simulator::softwareInterrupt] (Runtime call with unaligned stack!)
Build version: mozilla-central revision 5a5c74e9ccc0+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm
Runtime options: --fuzzing-safe --arm-hwcap=vfp --ion-eager

Testcase:

let lfPreamble = `
assertEq = function(a,b) {
  try { print(a); print(b); } catch(exc) {}
}
`;
evaluate(lfPreamble, {});
var a = [1, 2, 3];
var b = [4, 5, 6];
function testFold() {
    for (var i = 0; i < 10; i++) {
        var x = a[i];
        var z = b[i];
        if (((x / x) | 0) < 3) assertEq(x !== z, true);
    }
}
for (var i = 0; i < 10; i++) testFold();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  js::jit::Simulator::softwareInterrupt (this=0xf6e54000, instr=0xf57fc974) at js/src/jit/arm/Simulator-arm.cpp:2616
#1  0x084b4956 in js::jit::Simulator::decodeType7 (this=0xf6e54000, instr=0xf57fc974) at js/src/jit/arm/Simulator-arm.cpp:3875
#2  0x084b2c5a in js::jit::Simulator::instructionDecode (this=0xf6e54000, instr=0xf57fc974) at js/src/jit/arm/Simulator-arm.cpp:4855
#3  0x084b62ba in js::jit::Simulator::execute<false> (this=0xf6e54000) at js/src/jit/arm/Simulator-arm.cpp:4910
#4  js::jit::Simulator::callInternal (this=0xf6e54000, entry=0x55562538 "\360O-\351\004\320M\342\020\212-\355\r\200\240\341h\220\235\345\r\260\240\341t\240\235", <incomplete sequence \345>) at js/src/jit/arm/Simulator-arm.cpp:4990
#5  0x084b6391 in js::jit::Simulator::call (this=<optimized out>, entry=<optimized out>, argument_count=<optimized out>) at js/src/jit/arm/Simulator-arm.cpp:5073
#6  0x0833be06 in EnterJit (cx=cx@entry=0xf6e1c800, state=..., code=0x5556b098 "\004\340-\345\004") at js/src/jit/Jit.cpp:101
[...]
#17 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9465
eax	0x8a383b4	144933812
ebx	0x8a36ff4	144928756
ecx	0xf7d92864	-136763292
edx	0x888c0ae	143179950
esi	0xf6e54000	-152748032
edi	0x8a36ff4	144928756
ebp	0xffffce48	4294954568
esp	0xffffcdc0	4294954432
eip	0x84b48df <js::jit::Simulator::softwareInterrupt(js::jit::SimInstruction*)+2191>
=> 0x84b48df <js::jit::Simulator::softwareInterrupt(js::jit::SimInstruction*)+2191>:	movl   $0x0,0x0
   0x84b48e9 <js::jit::Simulator::softwareInterrupt(js::jit::SimInstruction*)+2201>:	ud2

Comment 115

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ __aeabi_idivmod]
Build version: mozilla-central revision 5a5c74e9ccc0+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm
Runtime options: --fuzzing-safe --arm-hwcap=vfp --ion-offthread-compile=off min.js

Testcase:

var lfRandom = 0;
while (true) {
    lfRunTypeId = 4 + (lfRandom % 2);
}

Backtrace:

received signal SIGFPE, Arithmetic exception.
0x0848f647 in __aeabi_idivmod (x=2, y=0) at js/src/jit/arm/Simulator-arm.cpp:55
#0  0x0848f647 in __aeabi_idivmod (x=2, y=0) at js/src/jit/arm/Simulator-arm.cpp:55
#1  0x084b44d6 in js::jit::Simulator::softwareInterrupt (this=0xf6e54000, instr=0xf57fc8d4) at js/src/jit/arm/Simulator-arm.cpp:2639
#2  0x084b4956 in js::jit::Simulator::decodeType7 (this=0xf6e54000, instr=0xf57fc8d4) at js/src/jit/arm/Simulator-arm.cpp:3875
#3  0x084b2c5a in js::jit::Simulator::instructionDecode (this=0xf6e54000, instr=0xf57fc8d4) at js/src/jit/arm/Simulator-arm.cpp:4855
#4  0x084b62ba in js::jit::Simulator::execute<false> (this=0xf6e54000) at js/src/jit/arm/Simulator-arm.cpp:4910
#5  js::jit::Simulator::callInternal (this=0xf6e54000, entry=0x5a257538 "...", <incomplete sequence \345>) at js/src/jit/arm/Simulator-arm.cpp:4990
#6  0x084b6391 in js::jit::Simulator::call (this=<optimized out>, entry=<optimized out>, argument_count=<optimized out>) at js/src/jit/arm/Simulator-arm.cpp:5073
#7  0x08262432 in EnterBaseline (data=..., cx=0xf6e1c800) at js/src/jit/BaselineJIT.cpp:161
#8  js::jit::EnterBaselineAtBranch (cx=0xf6e1c800, fp=0xf57c1018, pc=0xf5601946 "...") at js/src/jit/BaselineJIT.cpp:236
#9  0x081a7133 in Interpret (cx=0xf6e1c800, state=...) at js/src/vm/Interpreter.cpp:2182
[...]
#19 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9465
eax	0x2	2
ebx	0x5a25f9b4	1512438196
ecx	0x49	73
edx	0x0	0
esi	0xf6e54000	-152748032
edi	0xf57fc8d4	-176174892
ebp	0xffffcc68	4294954088
esp	0xffffcc68	4294954088
eip	0x848f647 <__aeabi_idivmod(int, int)+7>
=> 0x848f647 <__aeabi_idivmod(int, int)+7>:	idivl  0xc(%ebp)
   0x848f64a <__aeabi_idivmod(int, int)+10>:	pop    %ebp


This looks like the previously reported issue and hence I didn't see it earlier.

Comment 116

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

Comment on attachment 8991288 [details] [diff] [review]
Bug1438727.Rollup3.ApplyTo.5a5c74e9ccc0.patch

This rollup seems to be wrong; Cancelling f?s for now, will return to this later.

Comment 117

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

Attachment: [Part 22] Teach Baseline Inspector to infer binary arith cache types

Thanks for the great feedback Jan; As you realized I can actually simplify heavily by looking
at the operation instead, and I get to recycle much more code.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=655e77fe92bec736ae69c562c55c7a62152ca665

Comment 118

[Jan de Mooij [:jandem]]

Comment on attachment 8992377 [details] [diff] [review]
[Part 22] Teach Baseline Inspector to infer binary arith cache types

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

Nice!

Comment 119

[Ted Campbell [:tcampbell]]

Comment on attachment 8990451 [details] [diff] [review]
[Part 10] Implement flexibleDivMod

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

We talked in-person and agreed to re-write the x86 in terms of MoveResolver and LiveRegSet to greatly simplify. Will re-review on final version.
A few nits in the other platforms, but they are largely good.

Comment 120

[Ted Campbell [:tcampbell]]

Comment on attachment 8987604 [details] [diff] [review]
[Part 16] Support shifts in CacheIR

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

Hooray!

Comment 121

[Ted Campbell [:tcampbell]]

Comment on attachment 8990836 [details] [diff] [review]
[Part 18] Add StringObject Concat

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

Looks good now. Let's drop the test case though. It is a little dicey for localization.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +2430,5 @@
> +
> +    allocator.discardStack(masm);
> +
> +    EmitRestoreTailCallReg(masm);
> +    masm.pushValue(lhs);

Add comment that this is for expression decompiler.

::: js/src/jit/VMFunctions.h
@@ +956,5 @@
> +bool
> +DoConcatStringObject(JSContext* cx, HandleValue lhs, HandleValue rhs,
> +                     MutableHandleValue res);
> +
> +extern const VMFunction DoConcatStringObjectInfo;

Add a comment that this is the tailcall version of trampoline.

Comment 122

[Ted Campbell [:tcampbell]]

Comment on attachment 8987608 [details] [diff] [review]
[Part 20] Reorder attachment order to get correct fallthrough behaviour

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

Let's rewrite this so that ICs are more stand-alone. We can order AttachDouble after AttachInt32/AttachDoubleWithInt32 so that specialized ones have a chance to match.

Comment 123

[Ted Campbell [:tcampbell]]

Comment on attachment 8983091 [details] [diff] [review]
[Part 21] Reorder attachment order to get correct fallthrough behaviour

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

This seems to be a duplicate of the thing I just commented on.

Comment 124

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

Attachment: [Part 1 Fixup] Dump opcodes as part of CACHEIR_SPEW

Comment 125

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

Attachment: [Part 0 Fixup] Expand Test Case coverage to have examples for all ICs

Comment 126

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

Attachment: [Part 22] Add in fuzzing test cases

Comment 127

[Ted Campbell [:tcampbell]]

Comment on attachment 8993439 [details] [diff] [review]
[Part 22] Add in fuzzing test cases

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

:)

Comment 128

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

Attachment: [Part 9] Implement flexibleDivMod

Comment 129

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

Attachment: [Part 10] Implement flexible{quotient,remainder}32

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6d9ac0da42cc2ced1a0490f2caceb3a08f45f2c&selectedJob=189031838

Comment 130

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

Attachment: [Part 9] Implement flexibleDivMod

One extra note here is the use of setupUnalignedABICall in the ARM macro asssmebler 
(ditto for inbound part 10)

Comment 131

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

Attachment: [Part 10] Implement flexible{quotient,remainder}32

Comment 132

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

Attachment: Rollup for fuzzing. Apply to d94abfaa5992

Comment 133

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

Comment on attachment 8993662 [details]
Rollup for fuzzing. Apply to d94abfaa5992

Thanks so much for all the fuzz testing. It's been truly excellent. 

Another round?

Comment 134

[Ted Campbell [:tcampbell]]

Comment on attachment 8993659 [details] [diff] [review]
[Part 9] Implement flexibleDivMod

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

Great. Much cleaner.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp
@@ +321,5 @@
> +//
> +// 	[PUSH] Preserve registers
> +// 	[MOVE] Generate moves to specific registers
> +//
> +//  [DIV] Input: { regForRhs, EAX }

There are some tab/space issues here

@@ +343,5 @@
> +    // ebx is chosen arbitrarily, and will be preserved if necessary. 
> +    Register regForRhs = (rhs == eax || rhs == edx) ? ebx : rhs;
> +
> +    // Add registers we will be clobbering as live, but
> +    // also remove the set we cannot restore.

s/cannot/do not/

Comment 135

[Ted Campbell [:tcampbell]]

Comment on attachment 8993660 [details] [diff] [review]
[Part 10] Implement flexible{quotient,remainder}32

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

Great

Comment 136

[Christian Holler (:decoder)]

Comment on attachment 8993662 [details]
Rollup for fuzzing. Apply to d94abfaa5992

As discussed on IRC, I couldn't find any more failures except an isExceptionPending assertion that is likely OOM related and not directly caused by the patch. I don't have a working test for that right now, so we shouldn't block based on that.

Comment 137

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

Attachment: [Part 20] Rationalize stub attachment logic

Better comments and more explicit guards about attachment order to ensure the
stubs we want are attached where we want them.

(Whoops, I missed this on Friday! Was included in the rollup though)

Comment 138

[Ted Campbell [:tcampbell]]

Comment on attachment 8994590 [details] [diff] [review]
[Part 20] Rationalize stub attachment logic

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

Great. This seems far less brittle if we change any of these ICs in the future.

::: js/src/jit/CacheIR.cpp
@@ +5113,5 @@
>          return true;
> +
> +    // This attempt must come after tryAttachDoubleWithInt32
> +    // and tryAttachInt32, as it will attach for those cases
> +    if (tryAttachDouble())

This feel much more honest.

@@ +5230,5 @@
> +        return false;
> +
> +    // These ICs will failure() if result can't be encoded in an Int32:
> +    // If sample result is not Int32, we should avoid IC.
> +    if (!res_.isInt32())

Much clearer!

@@ +5303,5 @@
> +    // Only Addition
> +    if (op_ != JSOP_ADD)
> +        return false;
> +
> +    // Checck guards

s/Checck/Check/

Comment 139

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

Comment on attachment 8993662 [details]
Rollup for fuzzing. Apply to d94abfaa5992

I'd say f+ on this too. :)

Comment 140

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3e3e15772c
[Part 0] Add test case for binary arithmetic operations r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/50d88eac66ca
[Part 1] Implement a subset of JSOP_ADD in CacheIR r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc58162b8a3a
[Part 2] Implement a subset of JSOP_SUB in CacheIR r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/e86f52052359
[Part 3] Implement ADD+SUB for Boolean + Double|Int32 r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4df17b00a7e
[Part 4] Implement CacheIR IC for bitwise operations on Int32s r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1d2526dc10
[Part 5] Implement branchMul32 in MacroAssembler r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/d63f8f13573a
[Part 6] Allow allocateFixedRegister to spill in order to acquire a fixed register r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5a8676e1e8
[Part 7] Clarify restrictions on imul and remove un-needed restriction r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/4598c8290ae4
[Part 8] Implement JSOP_MUL in CacheIR r=tcampbell

Comment 141

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

https://hg.mozilla.org/mozilla-central/rev/1e3e3e15772c
https://hg.mozilla.org/mozilla-central/rev/50d88eac66ca
https://hg.mozilla.org/mozilla-central/rev/cc58162b8a3a
https://hg.mozilla.org/mozilla-central/rev/e86f52052359
https://hg.mozilla.org/mozilla-central/rev/e4df17b00a7e
https://hg.mozilla.org/mozilla-central/rev/8e1d2526dc10
https://hg.mozilla.org/mozilla-central/rev/d63f8f13573a
https://hg.mozilla.org/mozilla-central/rev/0f5a8676e1e8
https://hg.mozilla.org/mozilla-central/rev/4598c8290ae4

Comment 142

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4545c5afeb75
[Part 9] Implement flexibleDivMod r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ff1059472b
[Part 10] Implement flexible{quotient,remainder}32 r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b25886cfb64
[Part 11] Implement JSOP_DIV in CacheIR r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb598e5f90c
[Part 12] Implement JSOP_MOD in CacheIR r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/8127fbd4988d
[Part 13] Handle Double+Int32 bitwise operations r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2bd712cb42b
[Part 14] Support Double DIV and Double MOD r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/ede13cd0e89a
[Part 15] Implement flexible shift macroassembler helpers r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e257791eccf
[Part 16] Support shifts in CacheIR r=tcampbell

Comment 143

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4545c5afeb75
https://hg.mozilla.org/mozilla-central/rev/b0ff1059472b
https://hg.mozilla.org/mozilla-central/rev/5b25886cfb64
https://hg.mozilla.org/mozilla-central/rev/7cb598e5f90c
https://hg.mozilla.org/mozilla-central/rev/8127fbd4988d
https://hg.mozilla.org/mozilla-central/rev/c2bd712cb42b
https://hg.mozilla.org/mozilla-central/rev/ede13cd0e89a
https://hg.mozilla.org/mozilla-central/rev/8e257791eccf

Comment 144

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b10798b1b6a8
[Part 17] Add String+String Concatenation to CacheIR r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f5665631cc8
[Part 18] Add StringObject Concat r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/536ba7deb483
[Part 19] Teach the Baseline inspector about CacheIR stubs for BinaryArith r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c561fbeb0dfc
[Part 20] Rationalize stub attachment logic r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad859c41412
[Part 21] Teach Baseline Inspector to infer binary arith cache types r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/e57b84d2fcfd
[Part 22] Add in fuzzing test cases r=tcampbell

Comment 145

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b10798b1b6a8
https://hg.mozilla.org/mozilla-central/rev/2f5665631cc8
https://hg.mozilla.org/mozilla-central/rev/536ba7deb483
https://hg.mozilla.org/mozilla-central/rev/c561fbeb0dfc
https://hg.mozilla.org/mozilla-central/rev/3ad859c41412
https://hg.mozilla.org/mozilla-central/rev/e57b84d2fcfd