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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla39
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.
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.
Attachment #8571181 - Flags: review?(hv1989)
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?
Attachment #8571181 - Flags: review?(hv1989)
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)
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.
(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.
Attachment #8571181 - Attachment is obsolete: true
Attachment #8571340 - Flags: review?(hv1989)
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!
Attachment #8571340 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/6f507d09cfec
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.