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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(1 file)

Other platforms, certainly other 64-bit platforms, should be able to follow this pattern, but right now I'm concentrating on x64 only.
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 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+
(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.
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
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.
https://hg.mozilla.org/mozilla-central/rev/9d6ef06a18ad
https://hg.mozilla.org/mozilla-central/rev/663a28f984ab
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: