Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aarch64 - slight improvement to ffs64 #8077

Closed
wants to merge 1 commit into from

Conversation

swalk-cavium
Copy link
Contributor

This adds the Aarch64 equivalent code that was seen at the bottom of commit 1c338be.
This takes advantage of the assert to save 2 instructions per instance.

The standard regression tests were run with 6 option sets. No new problems were observed.
The generated assembly code was as expected.

Before

0000000005e8b574 <_ZN4HPHP5ffs64Em>:
...
5e8b5e4: 52801382 mov w2, #0x9c // #156
5e8b5e8: 97dc48c9 bl 559d90c <_ZN4HPHP11assert_failEPKcS1_jS1_RKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE>
5e8b5ec: f94017a0 ldr x0, [x29,#40]
5e8b5f0: f100001f cmp x0, #0x0 //<<---
5e8b5f4: dac00000 rbit x0, x0 //<<---
5e8b5f8: dac01000 clz x0, x0 //<<---
5e8b5fc: 9a8007e0 csinc x0, xzr, x0, eq //<<---
5e8b600: 51000400 sub w0, w0, #0x1

After

0000000005e8b574 <_ZN4HPHP5ffs64Em>:
...
5e8b5e4: 52801382 mov w2, #0x9c // #156
5e8b5e8: 97dc48c9 bl 559d90c <_ZN4HPHP11assert_failEPKcS1_jS1_RKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE>
5e8b5ec: f94017a0 ldr x0, [x29,#40]
5e8b5f0: dac00000 rbit x0, x0 //<<---
5e8b5f4: dac01000 clz x0, x0 //<<---
5e8b5f8: f90023a0 str x0, [x29,#64]
5e8b5fc: f94023a0 ldr x0, [x29,#64]

@swalk-cavium
Copy link
Contributor Author

@max - Hi Max, Can you review?

Copy link
Contributor

@hhvm-bot hhvm-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mxw has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@hhvm-bot hhvm-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mxw is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@hhvm-bot hhvm-bot closed this in cd10ba7 Jan 25, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants