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

WebAssembly i32.popcnt + tests #1459

Merged
merged 1 commit into from Aug 23, 2016

Conversation

Krovatkin
Copy link
Collaborator

@Krovatkin Krovatkin commented Aug 17, 2016

interpreter and codegen implementations for i32.popcnt fyi: @commonlisp @arunetm

@cellulle @MikeHolman homeless PR looks for a nice review and branch to be adopted into

@MikeHolman
Copy link
Contributor

:shipit:

@MikeHolman
Copy link
Contributor

FYI @Cellule. You guys might want to work out how to handle merging this, since your int64 change has some of this already (implemented slightly differently)

@arunetm-zz
Copy link
Contributor

@MikeHolman can you please help to sign off this change on the signing tool as well.
@Cellule I can help with merging this change. Can this be merged directly to the WebAssembly branch as there could be conflicts with the int64 changes.

@MikeHolman
Copy link
Contributor

@arunetm Done.

<default>
<files>i32_popcnt.js</files>
<baseline>i32_popcnt.baseline</baseline>
<compile-flags>-on:Wasm -maic:0</compile-flags>
Copy link
Contributor

Choose a reason for hiding this comment

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

this test is not needed as rl.exe will run a the other one with -forcenative

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Cellule right :-) let me update the PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@Cellule
Copy link
Contributor

Cellule commented Aug 18, 2016

The int64 change will come later so I will take care of resolving conflicts, it should not be too bad as I hadn't done the codegen part.

@Krovatkin
Copy link
Collaborator Author

@MikeHolman @Cellule, this is something i think i should prlly mention.
@commonlisp expressed a concern that moving popcnt to Math might interfere with inlining.
I ran a small test to check if it gets inlined in the only caller of popcnt in Chakra uint CharBitvec::Count() const and it seems to be the case by just looking at disassembly for CharBitvec::Count()

@MikeHolman
Copy link
Contributor

@Krovatkin I wouldn't worry about inlining, LTCG should take care of this just fine. But still good to hear confirmation.

refining asserts, removing nop

fixing whitespace

adding a copyright header

popcnt32 rename

 remove extern

moving popcnt32 to math

removing codegen test
@Cellule
Copy link
Contributor

Cellule commented Aug 18, 2016

LGTM

@chakrabot chakrabot merged commit e51083a into chakra-core:WebAssembly Aug 23, 2016
chakrabot pushed a commit that referenced this pull request Aug 23, 2016
Merge pull request #1459 from Krovatkin:wasm_popcnt32

interpreter and codegen implementations for i32.popcnt fyi: @commonlisp @arunetm

@cellulle @MikeHolman homeless PR looks for a nice review and branch to be adopted into
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.

None yet

6 participants