Closed
Bug 1215999
Opened 9 years ago
Closed 9 years ago
ARM64: Remove unnecessary float registers definitions.
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: victorcarlquist, Unassigned)
Details
Attachments
(1 file, 2 obsolete files)
2.54 KB,
patch
|
victorcarlquist
:
review+
|
Details | Diff | Splinter Review |
As Nicolas commented on bug 1211150 comment 10, the ReturnFloatReg and ScratchFloatReg seems to be aliases of ScratchDoubleReg and ReturnDoubleReg, so we could remove them or, maybe, set them with ScratchDoubleReg and ReturnDoubleReg respectively. We should do the same with the FloatReg0 and FloatReg1.
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8676466 -
Flags: review?(jolesen)
Comment 2•9 years ago
|
||
Comment on attachment 8676466 [details] [diff] [review] Patch Review of attachment 8676466 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I don't follow what is happening here. It's true that FloatReturnReg and DoubleReturnReg are aliasing each other, but they don't have the same size. One is 32 bits and the other is 64 bits. Similarly, v0 is a 128-bit register whose low 64 bits alias d0.
Attachment #8676466 -
Flags: review?(jolesen)
Comment 3•9 years ago
|
||
The FloatReg0 definition is erroneous and causes breakage on ARM64 tests, since it is defined as s0 but used as d0. In this case, "Float" means "Double", and "Float32" means "Single". They are named pretty badly. FloatReg1 cannot be ScratchFloatReg, though, since that may be independently acquired within the MacroAssembler at any point. In the context of this patch, v0 is an integer register code such that s0 == d0 == v0.
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•9 years ago
|
||
(In reply to Sean Stangl [:sstangl] from comment #3) > The FloatReg0 definition is erroneous and causes breakage on ARM64 tests, > since it is defined as s0 but used as d0. In this case, "Float" means > "Double", and "Float32" means "Single". They are named pretty badly. > > FloatReg1 cannot be ScratchFloatReg, though, since that may be independently > acquired within the MacroAssembler at any point. > > In the context of this patch, v0 is an integer register code such that s0 == > d0 == v0. Thanks, Sean. I can't find any uses of ReturnFloatReg with grep. Is there some kind of macro trickery in play, or can we delete it?
Comment 5•9 years ago
|
||
Same for ScratchFloatReg: It's unused.
Reporter | ||
Comment 6•9 years ago
|
||
Okay, thanks, I'll fix it.
Comment 7•9 years ago
|
||
Thanks, Victor. I assume that the bad FloatReg0 definition is the cause of these assertions: dist/bin/js ../js/src/jit-test/tests/basic/mod.js Assertion failure: !move.from().aliases(move.to()), at /Users/jolesen/gecko-dev/js/src/jit/MoveResolver.cpp:269 Segmentation fault: 11
Reporter | ||
Comment 8•9 years ago
|
||
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #7) > Thanks, Victor. > > I assume that the bad FloatReg0 definition is the cause of these assertions: > dist/bin/js ../js/src/jit-test/tests/basic/mod.js > Assertion failure: !move.from().aliases(move.to()), at > /Users/jolesen/gecko-dev/js/src/jit/MoveResolver.cpp:269 > Segmentation fault: 11 Yep!
Reporter | ||
Comment 9•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8676552 -
Flags: review?(jolesen)
Comment 10•9 years ago
|
||
Comment on attachment 8676552 [details] [diff] [review] Patch Review of attachment 8676552 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks! ::: js/src/jit/arm64/SharedICRegisters-arm64.h @@ +49,5 @@ > // register. In ARM code emission, we do not clobber BaselineTailCallReg > // since we keep the return address for calls there. > > +static constexpr FloatRegister FloatReg0 = { FloatRegisters::v0, FloatRegisters::Double }; > +static constexpr FloatRegister FloatReg1 = { FloatRegisters::v1, FloatRegisters::Double }; Since these registers are used as 64-bit doubles, I would prefer to use 'd0' and 'd1' instead of 'v0' and 'v1'. They have the same numerical value.
Attachment #8676552 -
Flags: review?(jolesen) → review+
Reporter | ||
Comment 11•9 years ago
|
||
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #10) > Since these registers are used as 64-bit doubles, I would prefer to use 'd0' > and 'd1' instead of 'v0' and 'v1'. They have the same numerical value. Ok, thanks for your review.
Reporter | ||
Comment 12•9 years ago
|
||
Attachment #8676466 -
Attachment is obsolete: true
Attachment #8676552 -
Attachment is obsolete: true
Attachment #8677128 -
Flags: review+
Reporter | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22e971f69c5b
Reporter | ||
Comment 14•9 years ago
|
||
Try is green, so I am setting the "checkin-needed" keyword. Thanks!
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/66dae447c298
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/66dae447c298
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•