Closed
Bug 1272014
Opened 8 years ago
Closed 8 years ago
Masm abstractions for ctz64 and clz64 (on x64)
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
Details
Attachments
(1 file)
2.83 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
Other platforms, certainly other 64-bit platforms, should be able to follow this pattern, but right now I'm concentrating on x64 only.
Assignee | ||
Comment 1•8 years ago
|
||
No test cases yet, because the 64-bit support in the baseline compiler is too feeble still. Hence not tested. But the code follows the 32-bit variants.
Attachment #8751328 -
Flags: feedback?(bbouvier)
Comment 2•8 years ago
|
||
Comment on attachment 8751328 [details] [diff] [review] bug1272014-ctz64-clz64.patch Review of attachment 8751328 [details] [diff] [review]: ----------------------------------------------------------------- Let's make this a r+, as I've tried it locally on my machine and it works great on my test cases (adding them in bug 1271972). Thanks! ::: js/src/jit/MacroAssembler.h @@ +812,5 @@ > // knownNotZero may be true only if the src is known not to be zero. > inline void clz32(Register src, Register dest, bool knownNotZero) DEFINED_ON(x86_shared); > inline void ctz32(Register src, Register dest, bool knownNotZero) DEFINED_ON(x86_shared); > > + inline void clz64(Register64 src, Register64 dest, bool knownNotZero) DEFINED_ON(x64); As long as we don't have range analysis on int64, I think knownNotZero will always be false. So feel free to remove it, or not (I tend to add features only when they are actually used, to avoid dead code and the false sentiment of "things already implemented are supposed to work").
Attachment #8751328 -
Flags: feedback?(bbouvier) → review+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #2) > Comment on attachment 8751328 [details] [diff] [review] > bug1272014-ctz64-clz64.patch > > As long as we don't have range analysis on int64, I think knownNotZero will > always be false. So feel free to remove it. That's sensible; will do so. Will land this first thing in the morning.
Comment 4•8 years ago
|
||
Just as a heads up in case you didn't notice it, the version of the patch you pushed to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f4a08d19293 has the wrong bug number: > Bug 1272015 - Masm code for 64-bit bit counting. r=bbouvier
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/663a28f984ab3418b6ccefe32e17502bb511c437 Bug 1272014 - make a correction that disappeared in a merge. r=bbouvier
Assignee | ||
Comment 7•8 years ago
|
||
When I messed up the bug number on the first landing the sheriff landed the patch that was attached to the bug when he was cleaning up, but that patch did not remove the knownNotZero parameter. So the second patch does that.
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9d6ef06a18ad https://hg.mozilla.org/mozilla-central/rev/663a28f984ab
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•