1137291 - Atomic primitives for asm.js on x86-32 clobber an input register

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

At present compareExchange and atomicBinop for asm.js on x86-32 clobber the "pointer" input register in the address calculation; I discovered this today when I was implementing an optimization that (apparently) allowed the register allocator reuse that register through a loop instead of reloading it on every iteration.

This is easily fixed by introducing a temp and that may be the expedient fix too.

Comment 1

[Luke Wagner [:luke]]

Right, as the general regalloc contract, input registers cannot be modified.  I've made this mistake before and try to look out for it... but fail apparently.

Comment 2

[Lars T Hansen [:lth]]

Attachment: Expedient fix - introduce a temp on x86-32

All this does is introduce a temp on x86-32 but the patch looks large since it requires moving some code from a shared file to a architecture files.

I am working on misc optimizations for this code anyway and will look into whether the temp can be avoided (if the code looks bad); at the moment I'm just going for correctness.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8571181 [details] [diff] [review]
Expedient fix - introduce a temp on x86-32

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

There is something clever we do to make sure we have less duplicated code (like you will introduce here):

Always emit BogusTemp for the second temp in the constructor. (So not as an argument in the constructor).

Keep:
LIRGeneratorX86Shared::visitAsmJSCompareExchangeHeap
and call "lowerAsmJSCompareExchangeHeap(lir)" after creating the lir.
LIRGeneratorX86Shared::visitAsmJSAtomicBinopHeap
and call "lowerAsmJSAtomicBinopHeap(lir)" after creating the lir.

Add:
LIRGeneratorX64::lowerAsmJSCompareExchangeHeap(LAsmJSCompareExchangeHeap *lir ...) {
    defineFixed(lir, ins, LAllocation(AnyRegister(eax)));
}
LIRGeneratorX86::lowerAsmJSCompareExchangeHeap(LAsmJSCompareExchangeHeap *lir ...) {
    ins->setTemp(1, ...);
    defineFixed(lir, ins, LAllocation(AnyRegister(eax)));
}
LIRGeneratorX64::lowerAsmJSAtomicBinopHeap(LAsmJSCompareExchangeHeap *lir ...) {
    ...
}
LIRGeneratorX86::lowerAsmJSAtomicBinopHeap(LAsmJSCompareExchangeHeap *lir ...) {
    ...
}


This keeps the x86shared code mostly shared, except for the lowering part, which can do platform specific stuff.
Can you do it like that?

Comment 4

[Hannes Verschore [:h4writer]]

Oops I have the dependency of LIRGeneratorX86 and LIRGeneratorX86Shared the other way around.

In that case can you do:
rename LIRGeneratorX86Shared::visitAsmJSCompareExchangeHeap to LIRGeneratorX86Shared::lowerAsmJSCompareExchangeHeap

And add temp2 variable to the arguments of that function.

Add 
LIRGeneratorX86::visitAsmJSCompareExchangeHeap
LIRGeneratorX64::visitAsmJSCompareExchangeHeap
where in the body you call "lowerAsmJSCompareExchangeHeap" with the correct temp2 variable in the arguments. (so bogus for 64 bit and an actual temp for 32 bit)

Comment 5

[Lars T Hansen [:lth]]

I'm not so sure.  For bug 1138348 and bug 1077036 the 32-bit and 64-bit versions will be specialized and will diverge a lot, probably (the code I have for bug 1138348 already looks pretty different and it is much smaller).  Arguably they should have been different from the outset.

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Lars T Hansen [:lth] from comment #5)
> I'm not so sure.  For bug 1138348 and bug 1077036 the 32-bit and 64-bit
> versions will be specialized and will diverge a lot, probably (the code I
> have for bug 1138348 already looks pretty different and it is much smaller).
> Arguably they should have been different from the outset.

I agree that when bug 1138348 or bug 1077036 diverge the x86/x64 visit functions too much, those bugs should separate them. Now for this correctness change it is not needed and would be cleaner/smaller to do the 'lower' way.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Expedient fix - introduce a temp on x86-32, v2

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8571340 [details] [diff] [review]
Expedient fix - introduce a temp on x86-32, v2

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

Awesome, thanks!

Comment 9

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f507d09cfec

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6f507d09cfec