Closed Bug 1072083 Opened 10 years ago Closed 8 years ago

visitSimdValueX4 doesn't need a tempCopy

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

x86_64
Linux
defect

Tracking

()

RESOLVED INVALID

People

(Reporter: mjrosenb, Unassigned)

Details

Attachments

(1 file)

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 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+
Benjamin, can this land, or is it long outdated?
Flags: needinfo?(bbouvier)
Priority: -- → P5
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.

Attachment

General

Created:
Updated:
Size: