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

sse4.2: first attempt at implementing mm_cmpestra #280

Closed

Conversation

masterchef2209
Copy link
Member

sse4.2: first attempt at implementing mm_cmpestra

@masterchef2209
Copy link
Member Author

please tell me if I am going in right direction in implementation of the function and generating test cases.
Thank You

@masterchef2209 masterchef2209 requested a review from nemequ May 9, 2020 22:04
Copy link
Member

@nemequ nemequ left a comment

Choose a reason for hiding this comment

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

This is a good start. A lot of the issues I see right away are relatively minor, but hopefully they'll make the code a little easier to understand, which should make it easier to optimize in the future.

simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
test/x86/sse4.2.c Show resolved Hide resolved
test/x86/sse4.2.c Outdated Show resolved Hide resolved
@masterchef2209
Copy link
Member Author

Thank You for the review, it will take some time to go through all of this :)

Copy link
Collaborator

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

simde/x86/sse4.2.h Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

simde/x86/sse4.2.h Outdated Show resolved Hide resolved
@mr-c
Copy link
Collaborator

mr-c commented May 11, 2020

That is odd, Travis CI hasn't seen the latest commit ..

Copy link
Member

@nemequ nemequ left a comment

Choose a reason for hiding this comment

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

Excellent progress! I added some more comments, but don't get discouraged. These functions are definitely a bit of a pani to implement (which is a big part of why they're not implemented yet), and you're doing really well. I just want to be a bit pedantic about this first function since it's probably going to be a model for the others.

I've tried to explain the reasons for each change, but if there is anything you're not clear on please ask.

simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
simde/x86/sse4.2.h Outdated Show resolved Hide resolved
@nemequ
Copy link
Member

nemequ commented May 15, 2020

The implementation looks good to me. As soon as you have the tests done we can merge it.

@masterchef2209
Copy link
Member Author

The implementation looks good to me. As soon as you have the tests done we can merge it.

I am working on it, hopefully I will be able to finish it by today.
Thanks

@nemequ
Copy link
Member

nemequ commented May 17, 2020

Closing in favor of #295

@nemequ nemequ closed this May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants