Closed
Bug 1137291
Opened 10 years ago
Closed 10 years ago
Atomic primitives for asm.js on x86-32 clobber an input register
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Blocks: shared-array-buffer
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8571181 -
Attachment is obsolete: true
Attachment #8571340 -
Flags: review?(hv1989)
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•