Skip to content

Correct lzcnt results - #1108

Merged
Stephan T. Lavavej (StephanTLavavej) merged 11 commits into
microsoft:masterfrom
AlexGuteniev:lzcnt
Aug 1, 2020
Merged

Correct lzcnt results#1108
Stephan T. Lavavej (StephanTLavavej) merged 11 commits into
microsoft:masterfrom
AlexGuteniev:lzcnt

Conversation

@AlexGuteniev

@AlexGuteniev Alex Guteniev (AlexGuteniev) commented Jul 29, 2020

Copy link
Copy Markdown
Contributor

Fixes #1103

@AlexGuteniev
Alex Guteniev (AlexGuteniev) requested a review from a team as a code owner July 29, 2020 19:52
Comment thread stl/inc/bit Outdated
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added the bug Something isn't working label Jul 30, 2020
@AlexGuteniev
Alex Guteniev (AlexGuteniev) marked this pull request as draft July 30, 2020 03:30

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great, just a couple of questions about naming. If you or Igor Zhukov (@fsb4000) can confirm that the tests for countl_zero fail on the affected processors before your change, and pass after, I think that should be sufficient for validation. (Note that you can compile a test program on one machine and just copy it to another if you statically link.)

Comment thread stl/inc/bit Outdated
Comment thread stl/inc/bit Outdated
Comment thread stl/inc/bit Outdated
@fsb4000

Copy link
Copy Markdown
Contributor

If you or Igor Zhukov (@fsb4000) can confirm that the tests for countl_zero fail on the affected processors before your change, and pass after, I think that should be sufficient for validation.

On my CPU(AMD FX8300) both versions(current master and the pull request) print same(correct) result:

test

@AlexGuteniev

Copy link
Copy Markdown
Contributor Author

I confirm the fix on pre AMB-LZCNT-POPCNT machine

@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks Igor Zhukov (@fsb4000) and Alex Guteniev (@AlexGuteniev)! I think this all makes sense - the FX-8300 worked because it physically supported Advanced Bit Manipulation, so the previous code said "oh, this doesn't support AVX2, we need to special-case 0" but otherwise used lzcnt which works. The test previously failed on Alex's even older processor which doesn't physically support lzcnt.

@barcharcraz

Copy link
Copy Markdown
Contributor

it sucks that we can't test this by modifying __isa_available before running the functions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

approved with a testing suggestion, I don't feel that suggestion should block merging this though.

Comment thread stl/inc/bit
Comment thread stl/inc/bit
@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks statementreply for noticing this bug, Alex Guteniev (@AlexGuteniev) for fixing it, and Igor Zhukov (@fsb4000) for helping to validate the fix! 😺 😸 🐈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<bit>: Concern about lzcnt used as bsr correctness

5 participants