Closed
Bug 1072083
Opened 10 years ago
Closed 8 years ago
visitSimdValueX4 doesn't need a tempCopy
Categories
(Core :: JavaScript Engine: JIT, defect, P5)
Tracking
()
RESOLVED
INVALID
People
(Reporter: mjrosenb, Unassigned)
Details
Attachments
(1 file)
3.32 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
http://hg.mozilla.org/mozilla-central/file/afc933adf723/js/src/jit/shared/Lowering-x86-shared.cpp#l378 gives the instruction a temp register, which is a copy of y, basically saying y(operand1) gets clobbered by the instruction. This makes sense, since it is used in the destination of: http://hg.mozilla.org/mozilla-central/file/afc933adf723/js/src/jit/shared/Lowering-x86-shared.cpp#l378 however, r0, r1, r2, and r3 are initially treated as float32, not float32x4, which means any operations on them will look only at slot0, nothing else. Assuming r1 = {y, 0, 0, 0} and r3 = {w, 0, 0, 0}, this means after executing |unpcklps r3, r1|, r1 will be r1 = {y, w, 0, 0}. Since the register allocator still believes r1 is a float32, it appears to still contain exactly the same value that it did before the instruction was executed. I'm tenatively asking for review on this. If the above logic isn't sound, then this should probably just be closed post haste.
Attachment #8494222 -
Flags: review?(benj)
Comment 1•10 years ago
|
||
Comment on attachment 8494222 [details] [diff] [review] removeCopyTemp-r0.patch Review of attachment 8494222 [details] [diff] [review]: ----------------------------------------------------------------- That makes sense as long as the input indeed is a float32. If one day, we want to optimize SimdExtractElementF, and says to reuse the input rather than defining a new output, this will break for cases like this: var x = SIMD.float32x4(1,2,3,4); var y = SIMD.float32x4(1, x.x, 2, 3); But in the meantime, we should be good to go.
Attachment #8494222 -
Flags: review?(benj) → review+
Comment 2•8 years ago
|
||
Benjamin, can this land, or is it long outdated?
Flags: needinfo?(bbouvier)
Priority: -- → P5
Comment 3•8 years ago
|
||
We actually have an invariant in jit code that the higher unused components of double/float32 have to be zero, for correctness of certain algorithms (e.g. compare to 0), so this is incorrect. Closing as invalid, since this is intended behavior.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(bbouvier)
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•