1090957 - IonMonkey: MIPS: Implement atomics

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Aleksandar Zlicic]

Attachment: Bug_1090957.patch

This is just a preliminary patch that fixes the build.

Comment 2

[Lars T Hansen [:lth]]

You may want to hold off on the non-stub implementations for a couple of days, there are more patches coming for asm.js and you could do them all in one go.

Comment 3

[Branislav Rankov [:rankov]]

Comment on attachment 8513490 [details] [diff] [review]
Bug_1090957.patch

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

::: js/src/jit/mips/MacroAssembler-mips.h
@@ +11,5 @@
>  
>  #include "jit/IonCaches.h"
>  #include "jit/IonFrames.h"
>  #include "jit/mips/Assembler-mips.h"
> +#include "jit/AtomicOp.h"

The includes are not proprely orederd. The make check will complain.

Comment 4

[Aleksandar Zlicic]

Attachment: Bug_1090957.patch

Comment 5

[Branislav Rankov [:rankov]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7aa70c02984

Comment 7

[Lars T Hansen [:lth]]

When you decide to flesh this out you should look at the patches on bug 979594 and bug 1073096, observe especially that the ARM changes on the latter bug change existing ARM functions in order to emit memory barriers for typed-array load and store, when appropriate.

Comment 8

[Aleksandar Zlicic]

Attachment: Bug_1090957-2.patch

Comment 9

[Branislav Rankov [:rankov]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1603cf22d4b6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1603cf22d4b6

Comment 11

[Aleksandar Zlicic]

Thanks for the heads-up, Lars.

I've read the comments thread for bug 979594 and bug 1073096, as well as the accompanying documents.
Since MIPS has (only) a 32-bit LL/SC instruction pair, implementation of 32 bit atomic operations is pretty straightforward, however for 8/16bit operations the best solution for achieving atomicity is to use a spinlock and 8/16bit non-atomic load/stores. This would require that 32 bits for spinlock are allocated and passed down to IonMonkey. Would these be allocated along with SharedArrayBuffer? Do you have any pointers/suggestions regarding this?

Comment 12

[Lars T Hansen [:lth]]

That's interesting.  My reading of the MIPS manual suggested that you should be able to use the 32-bit LL/SC instructions even for the smaller values.  The same technique is used on ARM.  Of course this leads to more potential contention but it is the only technique currently supported.  (This is the direct reason there are no atomics for 64-bit data in the spec.)

There's been talk that we should support a spinlock technique (for 64-bit), most likely there would have to be a single spinlock on each SharedArrayBuffer however, given how low-level the current API is.  For asm.js that would effectively be a global spinlock (since it has only one shared array) and it does not seem like a great idea, that lock could get very hot.

If I were you I would just use LL/SC with byte insertion, even if this leads to hairier machine code.  I will however open a new bug tracking this issue.  I'll cc you.

Comment 13

[Aleksandar Zlicic]

The alternative implementation will lead to machine code being much hairier indeed, but will also impose few extra conditions:
 - since memory locations accessed by LL/SC have to be aligned to 32bit boundary, arrays would also have to be aligned on 32bit boundary
 - additionally, array length (ie. memory allocated for array data) would have to be padded to a 4 byte multiple.

Could this be guaranteed?

Comment 14

[Lars T Hansen [:lth]]

Actually you don't quite need that, you only need the guarantee that the address of the byte of halfword rounded down to 4 bytes points to one word of valid memory that will not require an update at another byte or halfword within it while you're updating the byte or halfword you care about.

I believe that is guaranteed for all arrays, because a SharedArrayBuffer is always page-aligned and there are no headers etc on SharedTypedArray data, since they're just views on the SharedArrayBuffer.

Comment 15

[hev [:hev]]

I have implemented atomic operations in MIPS64. Although it looks is not very good, but works well. On MIPS, do a 8/16-bit atomic arithmetic operation need more than 2 scratch registers, obviously not enough, so push some volatile registers into stack and pop.

This is my implement:
https://github.com/heiher/gecko-dev/blob/mips64-dev/js/src/jit/mips64/MacroAssembler-mips64.cpp#L3752

Comment 16

[Lars T Hansen [:lth]]

(In reply to Heiher from comment #15)
> I have implemented atomic operations in MIPS64.

Nice!

An alternative for 1/2 byte ops would be to use the code that is coming for ARMv6, where a callout to C++ will be used to implement these ops, at least for the time being.  (Code size + number of temp regs needed + obsoleteness of ARMv6 combined to make that the best solution.)  But I see no reason to throw away working code for MIPS if you already have it.

Comment 17

[Aleksandar Zlicic]

Attachment: Bug_1090957-3.patch

This patch adds 32-bit atomics implementation only.
I'm still fixing a few bugs I made while implementing 8 and 16 bit atomics (these turned out to be even more complicated to implement than I previoously thought they would be), but I hope I will get them done soon.

Comment 18

[hev [:hev]]

Comment on attachment 8627219 [details] [diff] [review]
Bug_1090957-3.patch

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

May be you can refer my code. It's support 8 and 16-bit atomic load/store.

Link: https://github.com/heiher/gecko-dev/blob/f8b56bd463f17afb5568e47f301c47810740041e/js/src/jit/mips64/MacroAssembler-mips64.cpp#L3651

::: js/src/jit/mips/CodeGenerator-mips.cpp
@@ +2246,5 @@
> +
> +void
> +CodeGeneratorMIPS::memoryBarrier(MemoryBarrierBits barrier)
> +{
> +    masm.as_sync();

You should check barrier bits before insert sync instruction. if not, it will reduce the asm.js performance.

Comment 19

[Aleksandar Zlicic]

Thank You, Heiher, I will take a look at it.

Comment 20

[Lars T Hansen [:lth]]

Comment on attachment 8627219 [details] [diff] [review]
Bug_1090957-3.patch

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

Generally this looks roughly right but there are some bugs and I think the fences can be made cheaper, as detailed below.  A re-review is probably in order.

::: js/src/jit/mips/Assembler-mips.cpp
@@ +623,5 @@
>  
> +BufferOffset
> +Assembler::as_sync(void)
> +{
> +    return writeInst(op_special | ff_sync);

This emits a plain SYNC instruction, which is a completion barrier.  I think that is probably always correct, but a completion barrier is unnecessarily expensive, we only need ordering barriers.  From looking at the manual (table 3.7 under the SYNC instruction) this could be weakened to a SYNC 16, which is a full ordering barrier.  It would also be possible to specialize it further so that eg a StoreStore barrier, which is sometimes all that's needed, would be implemented as a SYNC 4.  The ARM code makes exactly this distinction, search jit/arm for "BarrierST", for example.

(I'm not a MIPS expert so I could have gotten this wrong but it seems plausible.  I would be fine with just going with a SYNC 16 everywhere for now, and skipping the complication of further optimization.)

::: js/src/jit/mips/CodeGenerator-mips.cpp
@@ +2033,5 @@
> +        masm.move32(Imm32(0), out);
> +        masm.ma_b(&rejoin, ShortJump);
> +        masm.bind(&goahead);
> +    }
> +    memoryBarrier(MembarFull);

You probably want this memoryBarrier() call before the move32 inside the switch (see comment below re synchronization in the oob case); the compareExchangeToTypedIntArray should take care of its own synchronization and should not need this one.

@@ +2060,5 @@
> +    BaseIndex srcAddr(HeapReg, ptrReg, TimesOne);
> +
> +    Label rejoin;
> +    uint32_t maybeCmpOffset = 0;
> +    if (mir->needsBoundsCheck()) {

This is correct for the time being, though we'll change this soon to throw an exception on out-of-bounds.  Just noting this so you'll be aware of it.

@@ +2067,5 @@
> +        Register out = ToRegister(ins->output());
> +        maybeCmpOffset = bo.getOffset();
> +        masm.ma_b(ptrReg, ScratchRegister, &goahead, Assembler::LessThan, ShortJump);
> +        // Offset is out of range. Load default values.
> +        masm.move32(Imm32(0), out);

The current spec requires a synchronization point before the result is generated so technically there is a bug here.  (Also see my comment above, on compareExchange.)  But it's probably a bug in the spec that it requires that synchronization point, and in any case this code will disappear when we switch to throwing an exception, so I wouldn't worry about it.

@@ +2246,5 @@
> +
> +void
> +CodeGeneratorMIPS::memoryBarrier(MemoryBarrierBits barrier)
> +{
> +    masm.as_sync();

Actually without the check it will reduce all TypedArray performance, asm.js or not, since the memoryBarrier() is inserted by regular TypedArray loads and stores (which will have empty barrier bit sets).

As I've noted on the implementation of as_sync(), you can specialize the barrier here based on which bits are set, using lighter-weight barriers when possible (eg SYNC 4 when only StoreStore is needed).  See CodeGeneratorARM::memoryBarrier() in jit/arm/CodeGenerator-arm.cpp, for example.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +3644,5 @@
> +        base = address.base;
> +    }
> +
> +    bind(&again);
> +    as_sync();

The sync could be moved before the bind, I think, though it might depend on the details of the memory model.

@@ +3650,5 @@
> +    ma_b(output, oldval, &done, NotEqual, ShortJump);
> +    ma_move(ScratchRegister, newval);
> +    as_sc(ScratchRegister, base, encodedOffset);
> +    ma_b(ScratchRegister, Imm32(0), &again, Equal, ShortJump);
> +    bind(&done);

Here we are probably missing a sync after the done.  I believe that MIPS32 has a weak memory model a la ARM, so it requires a StoreStore fence before the CompareExchange operation and a full fence after the operation.

The following page does not have information on MIPS, but it describes ARM and PPC in some detail and these should be similar to MIPS: http://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html.

If I understand the MIPS docs correctly it is not necessary to use a full SYNC fence, one can use lighter weight variants.  I've commented on this a couple of other places too.

@@ +3701,5 @@
> +        ma_xor(ScratchRegister, output, value);
> +        break;
> +    }
> +    as_sc(ScratchRegister, base, encodedOffset);
> +    ma_b(ScratchRegister, Imm32(0), &again, Equal, ShortJump);

Again missing a fence after the sequence.

Comment 21

[Branislav Rankov [:rankov]]

Comment on attachment 8627219 [details] [diff] [review]
Bug_1090957-3.patch

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

Just a few style comments from me.

::: js/src/jit/mips/Lowering-mips.cpp
@@ +584,5 @@
>  
>  void
>  LIRGeneratorMIPS::visitAsmJSCompareExchangeHeap(MAsmJSCompareExchangeHeap* ins)
>  {
> +    MDefinition *ptr = ins->ptr();

Move * next to the type. Here and other places. As per new code style.

::: js/src/jit/mips/MacroAssembler-mips.h
@@ +960,5 @@
>      /////////////////////////////////////////////////////////////////
>    public:
>      // The following functions are exposed for use in platform-shared code.
>  
> +

No need to add the new line here.

Comment 22

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS32-Implement-atomicExchange-operations.patch

Implement atomicExchange operations temporary, let build pass.
When mips64 merged, these atomic operations will moved to mips-shared from mips64 code, shared between mips32 and 64.

Comment 23

[Pulsebot]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f02ddcee9c9f

Comment 25

[hev [:hev]]

Attachment: Part 1: Implement atomics in CodeGenerator-mips-shared.

Thanks!

Comment 26

[hev [:hev]]

Attachment: Part 2: Implement atomics in Lowering-mips-shared.

Thanks!

Comment 27

[Lars T Hansen [:lth]]

Comment on attachment 8672517 [details] [diff] [review]
Part 2: Implement atomics in Lowering-mips-shared.

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

This is probably OK as it is, though you want to look at the comments below.

::: js/src/jit/mips-shared/Lowering-mips-shared.cpp
@@ +390,5 @@
> +
> +    // If the target is a floating register then we need a temp at the
> +    // CodeGenerator level for creating the result.
> +    //
> +    // Optimization opportunity (bug 1077317): We could do better by

Bug 1077317 is ARM-specific.  If there is a similar optimization opportunity on MIPS then file a separate bug for MIPS and update this part of the comment.  Otherwise kill this part of the comment.

@@ +460,5 @@
> +
> +    // The output may not be used but will be clobbered regardless,
> +    // so ignore the case where we're not using the value and just
> +    // use the output register as a temp.
> +

I think the preceding comment is specific to x86 (where we use the XCHG register), you should probably remove it.

@@ +512,5 @@
> +        return;
> +    }
> +
> +    LAtomicTypedArrayElementBinop* lir =
> +        new(alloc()) LAtomicTypedArrayElementBinop(elements, index, value, temp(), temp());

On ARM, we only need the second of these temps if there's an Uint32 case, so you may be able to avoid it here too.  (TBD).

Comment 28

[Lars T Hansen [:lth]]

Comment on attachment 8672516 [details] [diff] [review]
Part 1: Implement atomics in CodeGenerator-mips-shared.

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

I'm going to cancel the review for now.  The reason is that you have not implemented the many functions in the macro-assembler that support this code (atomicOr32, atomicFetchXor8SignExtend -- there are lots of them).  My recent experience with ARM is that it is important to get those right first, so that you can see what registers you will need in the code generator - I needed to add more registers to the API to handle the ll/sc idioms in the ARM back-end, see the pervasive use of "flagTemp" in the ARM code.  MIPS may be different if you have more temps available in the assembler, but I'd still like to see these macro-assembler functions first in order to judge the correctness of the present patch.

(Apart from that the patch looks OK to me so a re-review later should be quick.)

Comment 29

[hev [:hev]]

Comment on attachment 8647320 [details] [diff] [review]
0001-IonMonkey-MIPS32-Implement-atomicExchange-operations.patch

Cancel it, The latest implementation uses temp registers that allocated from LIR.

Comment 30

[hev [:hev]]

Attachment: Part 1: Add temp registers for mips atomics.

Comment 31

[hev [:hev]]

Attachment: Part 2: Implement atomics in MacroAssembler mips shared.

Comment 32

[hev [:hev]]

Attachment: Part 3: Implement atomics in MacroAssembler mips32.

Comment 33

[hev [:hev]]

Attachment: Part 4: Implement atomics in CodeGenerator.

Comment 34

[hev [:hev]]

Attachment: Part 5: Implement atomics in Lowering.

Comment 35

[hev [:hev]]

Attachment: Part 6: Implement atomics in MacroAssembler mips64.

This patch is based on Bug 1213746.

Comment 36

[Lars T Hansen [:lth]]

Comment on attachment 8683511 [details] [diff] [review]
Part 1: Add temp registers for mips atomics.

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

OK.  I actually think that it would actually be better if these constructors did not have a lot of optional values, but that there should instead be two constructors in that case: the constructor we already have, and a new constructor that takes the additional arguments (which would not be optional).  The reason I think that is that it would reduce the risk of calling these functions with too few arguments.  But I don't know if it makes a big difference and I'm not going to insist on it.

Comment 37

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #36)
> Comment on attachment 8683511 [details] [diff] [review]
> Part 1: Add temp registers for mips atomics.
> 
> Review of attachment 8683511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK.  I actually think that it would actually be better if these constructors
> did not have a lot of optional values, but that there should instead be two
> constructors in that case: the constructor we already have, and a new
> constructor that takes the additional arguments (which would not be
> optional).  The reason I think that is that it would reduce the risk of
> calling these functions with too few arguments.  But I don't know if it
> makes a big difference and I'm not going to insist on it.

I am however going to ask Hannes what he thinks about it, just to get a second opinion :)

Comment 38

[Lars T Hansen [:lth]]

Comment on attachment 8683514 [details] [diff] [review]
Part 3: Implement atomics in MacroAssembler mips32.

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

::: js/src/jit/mips32/MacroAssembler-mips32.h
@@ +755,5 @@
>      // The following functions are exposed for use in platform-shared code.
>  
>      template<typename T>
> +    void compareExchange8SignExtend(const T& mem, Register oldval, Register newval, Register valueTemp,
> +                                    Register offsetTemp, Register maskTemp, Register output)

Note these functions can be made private, they are only accessed from elsewhere within the assembler now.

Comment 39

[Lars T Hansen [:lth]]

Comment on attachment 8683519 [details] [diff] [review]
Part 6: Implement atomics in MacroAssembler mips64.

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

::: js/src/jit/mips64/MacroAssembler-mips64.h
@@ +772,5 @@
>      // The following functions are exposed for use in platform-shared code.
>  
>      template<typename T>
> +    void compareExchange8SignExtend(const T& mem, Register oldval, Register newval, Register valueTemp,
> +                                    Register offsetTemp, Register maskTemp, Register output)

Again, these can be made private I think.

Comment 40

[Lars T Hansen [:lth]]

Comment on attachment 8683513 [details] [diff] [review]
Part 2: Implement atomics in MacroAssembler mips shared.

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +809,5 @@
> +    as_andi(offsetTemp, addr, 3);
> +    asMasm().subPtr(offsetTemp, addr);
> +    as_sll(offsetTemp, offsetTemp, 3);
> +    ma_li(maskTemp, Imm32(UINT32_MAX >> ((4 - nbytes) * 8)));
> +    as_sllv(maskTemp, maskTemp, offsetTemp);

None of this work (and similar work below) is needed when nbytes=4, which is going to be a common case.  (In that case you probably will also not need most of the extra registers.)  It's OK with me if you want to leave this code the way it is, but if you do then I'd like you to file a followup bug to specialize this code and code higher up for nbytes=4, and you should reference that bug from a comment here.

@@ +813,5 @@
> +    as_sllv(maskTemp, maskTemp, offsetTemp);
> +
> +    bind(&again);
> +
> +    as_sync(0);

I think it's possible to use a cheaper barrier here.  Very definitely you can use sync 16 here and below, and it should be cheaper than sync 0, which has to wait for the memory system to perform pending transactions both times.

@@ +830,5 @@
> +            case 2:
> +                as_seh(output, output);
> +                break;
> +            case 4:
> +                as_sll(output, output, 0);

This is a NOP, do you need it?

@@ +966,5 @@
> +    as_andi(offsetTemp, addr, 3);
> +    asMasm().subPtr(offsetTemp, addr);
> +    as_sll(offsetTemp, offsetTemp, 3);
> +    ma_li(maskTemp, Imm32(UINT32_MAX >> ((4 - nbytes) * 8)));
> +    as_sllv(maskTemp, maskTemp, offsetTemp);

Same comment here as above, you can streamline this a lot for the case nbytes=4.

@@ +970,5 @@
> +    as_sllv(maskTemp, maskTemp, offsetTemp);
> +
> +    bind(&again);
> +
> +    as_sync(0);

Also here, sync 16 should be fine (and there may be further optimizations).

Comment 41

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #38)
> Comment on attachment 8683514 [details] [diff] [review]
> Part 3: Implement atomics in MacroAssembler mips32.
> 
> Review of attachment 8683514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips32/MacroAssembler-mips32.h
> @@ +755,5 @@
> >      // The following functions are exposed for use in platform-shared code.
> >  
> >      template<typename T>
> > +    void compareExchange8SignExtend(const T& mem, Register oldval, Register newval, Register valueTemp,
> > +                                    Register offsetTemp, Register maskTemp, Register output)
> 
> Note these functions can be made private, they are only accessed from
> elsewhere within the assembler now.

I now realize that's probably wrong.  If so then please ignore it :)

Comment 42

[Lars T Hansen [:lth]]

Comment on attachment 8683516 [details] [diff] [review]
Part 5: Implement atomics in Lowering.

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

::: js/src/jit/mips-shared/Lowering-mips-shared.cpp
@@ +390,5 @@
> +
> +    // If the target is a floating register then we need a temp at the
> +    // CodeGenerator level for creating the result.
> +    //
> +    // Optimization opportunity (bug 1077317): We could do better by

Bug 1077317 is an ARM-specific bug, you should not reference it here but you should file your own if you feel that that optimization is worthwhile on MIPS.

@@ +396,5 @@
> +    // to fit in an instruction.
> +
> +    const LAllocation newval = useRegister(ins->newval());
> +    const LAllocation oldval = useRegister(ins->oldval());
> +    LDefinition tempDef = LDefinition::BogusTemp();

Given that there are many temps I would suggest "uint32temp" as a better name than just the generic "tempDef", also in methods below.  (ARM needs the same fix.)

Comment 43

[hev [:hev]]

(In reply to Lars T Hansen [:lth] from comment #40)
> Comment on attachment 8683513 [details] [diff] [review]
> Part 2: Implement atomics in MacroAssembler mips shared.
> 
> Review of attachment 8683513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
> @@ +830,5 @@
> > +            case 2:
> > +                as_seh(output, output);
> > +                break;
> > +            case 4:
> > +                as_sll(output, output, 0);
> 
> This is a NOP, do you need it?
>
It might be not required here, sign extend to 64-bit register at above srlv instruction for 4-byte.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da2deb62849e
https://hg.mozilla.org/integration/mozilla-inbound/rev/92f45c98b930
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8324f9509f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b1e29fa2db
https://hg.mozilla.org/integration/mozilla-inbound/rev/f45ccb4ab2cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b64cc3528d9

Comment 45

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

Had to back this out for build failures.

Failing push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0b64cc3528d9
Errors:
13:27:35     INFO -  Unified_cpp_js_src22.o
13:27:39     INFO -  In file included from /home/worker/objdir-gecko/objdir/js/src/Unified_cpp_js_src19.cpp:2:0:
13:27:39     INFO -  /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp: In member function 'virtual void js::jit::LIRGeneratorARM::visitAtomicExchangeTypedArrayElement(js::jit::MAtomicExchangeTypedArrayElement*)':
13:27:39    ERROR -  /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:574:86: error: call of overloaded 'LAtomicExchangeTypedArrayElement(const js::jit::LUse&, const js::jit::LAllocation&, const js::jit::LAllocation&, js::jit::LDefinition&)' is ambiguous
13:27:39     INFO -           new(alloc()) LAtomicExchangeTypedArrayElement(elements, index, value, tempDef);
13:27:39     INFO -                                                                                        ^
13:27:39     INFO -  /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:574:86: note: candidates are:
13:27:39     INFO -  In file included from /home/worker/workspace/gecko/js/src/jit/LIR.h:1818:0,
13:27:39     INFO -                   from /home/worker/workspace/gecko/js/src/jit/Lowering.h:13,
13:27:39     INFO -                   from /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:10,
13:27:39     INFO -                   from /home/worker/objdir-gecko/objdir/js/src/Unified_cpp_js_src19.cpp:2:
13:27:39     INFO -  /home/worker/workspace/gecko/js/src/jit/shared/LIR-shared.h:5258:5: note: js::jit::LAtomicExchangeTypedArrayElement::LAtomicExchangeTypedArrayElement(const js::jit::LAllocation&, const js::jit::LAllocation&, const js::jit::LAllocation&, const js::jit::LDefinition&, const js::jit::LDefinition&, const js::jit::LDefinition&, const js::jit::LDefinition&)
13:27:39     INFO -       LAtomicExchangeTypedArrayElement(const LAllocation& elements, const LAllocation& index,
13:27:39     INFO -       ^
13:27:39     INFO -  /home/worker/workspace/gecko/js/src/jit/shared/LIR-shared.h:5250:5: note: js::jit::LAtomicExchangeTypedArrayElement::LAtomicExchangeTypedArrayElement(const js::jit::LAllocation&, const js::jit::LAllocation&, const js::jit::LAllocation&, const js::jit::LDefinition&)
13:27:39     INFO -       LAtomicExchangeTypedArrayElement(const LAllocation& elements, const LAllocation& index,
13:27:39     INFO -       ^
13:27:39     INFO -  In file included from /home/worker/objdir-gecko/objdir/js/src/Unified_cpp_js_src19.cpp:2:0:
13:27:39     INFO -  /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp: In member function 'virtual void js::jit::LIRGeneratorARM::visitAtomicTypedArrayElementBinop(js::jit::MAtomicTypedArrayElementBinop*)':
13:27:39    ERROR -  /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:596:87: error: call of overloaded 'LAtomicTypedArrayElementBinopForEffect(const js::jit::LUse&, const js::jit::LAllocation&, const js::jit::LAllocation&, js::jit::LDefinition)' is ambiguous
13:27:39     INFO -                                                                   /* flagTemp= */ temp());
13:27:39     INFO -                                                                                         ^
13:27:39     INFO -  /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:596:87: note: candidates are:
13:27:39     INFO -  In file included from /home/worker/workspace/gecko/js/src/jit/LIR.h:1818:0,
13:27:39     INFO -                   from /home/worker/workspace/gecko/js/src/jit/Lowering.h:13,
13:27:39     INFO -                   from /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:10,
13:27:39     INFO -                   from /home/worker/objdir-gecko/objdir/js/src/Unified_cpp_js_src19.cpp:2:
13:27:39     INFO -  /home/worker/workspace/gecko/js/src/jit/shared/LIR-shared.h:5384:5: note: js::jit::LAtomicTypedArrayElementBinopForEffect::LAtomicTypedArrayElementBinopForEffect(const js::jit::LAllocation&, const js::jit::LAllocation&, const js::jit::LAllocation&, const js::jit::LDefinition&, const js::jit::LDefinition&, const js::jit::LDefinition&, const js::jit::LDefinition&)
13:27:39     INFO -       LAtomicTypedArrayElementBinopForEffect(const LAllocation& elements, const LAllocation& index,
13:27:39     INFO -       ^

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa558b944b5
Backout jobs: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4aa558b944b5

Comment 46

[hev [:hev]]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #45)
> Had to back this out for build failures.
> 
> Failing push:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=0b64cc3528d9
> Errors:
> 13:27:35     INFO -  Unified_cpp_js_src22.o
> 13:27:39     INFO -  In file included from
> /home/worker/objdir-gecko/objdir/js/src/Unified_cpp_js_src19.cpp:2:0:
> 13:27:39     INFO - 
> /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp: In member
> function 'virtual void
> js::jit::LIRGeneratorARM::visitAtomicExchangeTypedArrayElement(js::jit::
> MAtomicExchangeTypedArrayElement*)':
> 13:27:39    ERROR - 
> /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:574:86: error:
> call of overloaded 'LAtomicExchangeTypedArrayElement(const js::jit::LUse&,
> const js::jit::LAllocation&, const js::jit::LAllocation&,
> js::jit::LDefinition&)' is ambiguous
> 13:27:39     INFO -           new(alloc())
> LAtomicExchangeTypedArrayElement(elements, index, value, tempDef);
> 13:27:39     INFO -                                                         
> ^
> 13:27:39     INFO - 
> /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:574:86: note:
> candidates are:
> 13:27:39     INFO -  In file included from
> /home/worker/workspace/gecko/js/src/jit/LIR.h:1818:0,
> 13:27:39     INFO -                   from
> /home/worker/workspace/gecko/js/src/jit/Lowering.h:13,
> 13:27:39     INFO -                   from
> /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:10,
> 13:27:39     INFO -                   from
> /home/worker/objdir-gecko/objdir/js/src/Unified_cpp_js_src19.cpp:2:
> 13:27:39     INFO - 
> /home/worker/workspace/gecko/js/src/jit/shared/LIR-shared.h:5258:5: note:
> js::jit::LAtomicExchangeTypedArrayElement::
> LAtomicExchangeTypedArrayElement(const js::jit::LAllocation&, const
> js::jit::LAllocation&, const js::jit::LAllocation&, const
> js::jit::LDefinition&, const js::jit::LDefinition&, const
> js::jit::LDefinition&, const js::jit::LDefinition&)
> 13:27:39     INFO -       LAtomicExchangeTypedArrayElement(const
> LAllocation& elements, const LAllocation& index,
> 13:27:39     INFO -       ^
> 13:27:39     INFO - 
> /home/worker/workspace/gecko/js/src/jit/shared/LIR-shared.h:5250:5: note:
> js::jit::LAtomicExchangeTypedArrayElement::
> LAtomicExchangeTypedArrayElement(const js::jit::LAllocation&, const
> js::jit::LAllocation&, const js::jit::LAllocation&, const
> js::jit::LDefinition&)
> 13:27:39     INFO -       LAtomicExchangeTypedArrayElement(const
> LAllocation& elements, const LAllocation& index,
> 13:27:39     INFO -       ^
> 13:27:39     INFO -  In file included from
> /home/worker/objdir-gecko/objdir/js/src/Unified_cpp_js_src19.cpp:2:0:
> 13:27:39     INFO - 
> /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp: In member
> function 'virtual void
> js::jit::LIRGeneratorARM::visitAtomicTypedArrayElementBinop(js::jit::
> MAtomicTypedArrayElementBinop*)':
> 13:27:39    ERROR - 
> /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:596:87: error:
> call of overloaded 'LAtomicTypedArrayElementBinopForEffect(const
> js::jit::LUse&, const js::jit::LAllocation&, const js::jit::LAllocation&,
> js::jit::LDefinition)' is ambiguous
> 13:27:39     INFO -                                                         
> /* flagTemp= */ temp());
> 13:27:39     INFO -                                                         
> ^
> 13:27:39     INFO - 
> /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:596:87: note:
> candidates are:
> 13:27:39     INFO -  In file included from
> /home/worker/workspace/gecko/js/src/jit/LIR.h:1818:0,
> 13:27:39     INFO -                   from
> /home/worker/workspace/gecko/js/src/jit/Lowering.h:13,
> 13:27:39     INFO -                   from
> /home/worker/workspace/gecko/js/src/jit/arm/Lowering-arm.cpp:10,
> 13:27:39     INFO -                   from
> /home/worker/objdir-gecko/objdir/js/src/Unified_cpp_js_src19.cpp:2:
> 13:27:39     INFO - 
> /home/worker/workspace/gecko/js/src/jit/shared/LIR-shared.h:5384:5: note:
> js::jit::LAtomicTypedArrayElementBinopForEffect::
> LAtomicTypedArrayElementBinopForEffect(const js::jit::LAllocation&, const
> js::jit::LAllocation&, const js::jit::LAllocation&, const
> js::jit::LDefinition&, const js::jit::LDefinition&, const
> js::jit::LDefinition&, const js::jit::LDefinition&)
> 13:27:39     INFO -       LAtomicTypedArrayElementBinopForEffect(const
> LAllocation& elements, const LAllocation& index,
> 13:27:39     INFO -       ^
> 
> backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa558b944b5
> Backout jobs:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=4aa558b944b5

Oh. sorry, I forget remove optional values in new constructors.

Comment 47

[hev [:hev]]

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

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47dda6e69a9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/06b8848a99f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8c999e1020
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd89b90689f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52a67af90b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/62147f799ef1

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47dda6e69a9f
https://hg.mozilla.org/mozilla-central/rev/06b8848a99f5
https://hg.mozilla.org/mozilla-central/rev/0d8c999e1020
https://hg.mozilla.org/mozilla-central/rev/ddd89b90689f
https://hg.mozilla.org/mozilla-central/rev/a52a67af90b2
https://hg.mozilla.org/mozilla-central/rev/62147f799ef1

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47dda6e69a9f
https://hg.mozilla.org/mozilla-central/rev/06b8848a99f5
https://hg.mozilla.org/mozilla-central/rev/0d8c999e1020
https://hg.mozilla.org/mozilla-central/rev/ddd89b90689f
https://hg.mozilla.org/mozilla-central/rev/a52a67af90b2
https://hg.mozilla.org/mozilla-central/rev/62147f799ef1

Comment 51

[Hannes Verschore [:h4writer]]

(In reply to Lars T Hansen [:lth] from comment #37)
> (In reply to Lars T Hansen [:lth] from comment #36)
> > Comment on attachment 8683511 [details] [diff] [review]
> > Part 1: Add temp registers for mips atomics.
> > 
> > Review of attachment 8683511 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > OK.  I actually think that it would actually be better if these constructors
> > did not have a lot of optional values, but that there should instead be two
> > constructors in that case: the constructor we already have, and a new
> > constructor that takes the additional arguments (which would not be
> > optional).  The reason I think that is that it would reduce the risk of
> > calling these functions with too few arguments.  But I don't know if it
> > makes a big difference and I'm not going to insist on it.
> 
> I am however going to ask Hannes what he thinks about it, just to get a
> second opinion :)

I think it is fine as-is. We will get runtime errors when a bogus-temp is used as real reg. I.e. we will get errors when the constructor is mis-used.