FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sse4.2: first attempt at implementing mm_cmpestra by masterchef2209 · Pull Request #280 · simd-everywhere/simde · GitHub

/ simde Public

sse4.2: first attempt at implementing mm_cmpestra - #280

Closed
masterchef2209 wants to merge 14 commits into
simd-everywhere:masterfrom
masterchef2209:hidayat/may10_1
ConversationCommits (14)ChecksFiles changed
Closed

sse4.2: first attempt at implementing mm_cmpestra#280
masterchef2209 wants to merge 14 commits into
simd-everywhere:masterfrom
masterchef2209:hidayat/may10_1

Conversation

masterchef2209 commented May 9, 2020

Copy link
Copy Markdown
Member

sse4.2: first attempt at implementing mm_cmpestra

masterchef2209 commented May 9, 2020

Copy link
Copy Markdown
Member Author

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

masterchef2209 requested a review from nemequ May 9, 2020 22:04
nemequ requested changes May 10, 2020
View reviewed changes

nemequ left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread test/x86/sse4.2.c
Comment thread test/x86/sse4.2.c Outdated

masterchef2209 commented May 10, 2020

Copy link
Copy Markdown
Member Author

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

mr-c requested changes May 11, 2020
View reviewed changes

mr-c left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Feedback from https://github.com/nemequ/simde/pull/280/checks?check_run_id=662415340#step:5:17

Comment thread simde/x86/sse4.2.h
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
mr-c requested changes May 11, 2020
View reviewed changes

mr-c left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Almost there! Another issue from https://github.com/nemequ/simde/pull/280/checks?check_run_id=662532291#step:5:66

Comment thread simde/x86/sse4.2.h Outdated

mr-c commented May 11, 2020

Copy link
Copy Markdown
Collaborator

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

nemequ requested changes May 11, 2020
View reviewed changes

nemequ left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
Comment thread simde/x86/sse4.2.h Outdated
nemequ reviewed May 13, 2020
View reviewed changes
Comment thread simde/x86/sse4.2.h Outdated
nemequ reviewed May 13, 2020
View reviewed changes
Comment thread simde/x86/sse4.2.h Outdated

nemequ commented May 15, 2020

Copy link
Copy Markdown
Member

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

masterchef2209 commented May 16, 2020

Copy link
Copy Markdown
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 commented May 17, 2020

Copy link
Copy Markdown
Member

Closing in favor of #295

nemequ closed this May 17, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

nemequ nemequ requested changes
rosbif rosbif left review comments
mr-c mr-c requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL