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

[Bazel/MSVC] Enable jpeg SIMD for MSVC #20537

Merged
merged 1 commit into from
Sep 8, 2018

Conversation

rongjiecomputer
Copy link
Contributor

No description provided.

meteorcloudy
meteorcloudy previously approved these changes Jul 11, 2018
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@tensorflowbutler
Copy link
Member

Nagging Assignee @martinwicke: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

meteorcloudy
meteorcloudy previously approved these changes Aug 7, 2018
@qlzh727 qlzh727 added the kokoro:force-run Tests on submitted change label Aug 10, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 10, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 10, 2018
@qlzh727 qlzh727 added the ready to pull PR ready for merge process label Aug 10, 2018
@gunan
Copy link
Contributor

gunan commented Aug 10, 2018

I cannot tell how, but the failures look legit.
This is what we see on windows:

ERROR: T:/tmp/bigvaudl/external/nasm/BUILD.bazel:8:1: undeclared inclusion(s) in rule '@nasm//:nasm':
this rule is missing dependency declarations for the following files included by 'external/nasm/stdlib/strlcpy.c':
  'external/nasm/config/msvc.h'

@rongjiecomputer could you conditionally add msvc.h to this list, maybe what will fix it?
https://github.com/tensorflow/tensorflow/blob/master/third_party/nasm.BUILD#L45
I think with the new headers, somehow different code paths are activated in nasm by some macros.

@rongjiecomputer
Copy link
Contributor Author

@gunan f22edf3 should fix the build error for nasm. Please trigger CI build again.

@gunan gunan added the kokoro:force-run Tests on submitted change label Aug 25, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 25, 2018
@rongjiecomputer
Copy link
Contributor Author

#21993 is going to break this PR again. Can we get this or that PR merged soon?

@perfinion
Copy link
Member

@rongjiecomputer the update to 2.0.0 needs to go in quickly cuz it fixes some CVEs in the current version. That version passes all the tests but I don't really know how to do the SIMD stuff on windows. It looks like 2.0.0 adds quite a bit of support tho so ideally could you rebase this on top of the 2.0.0 one to enable the windows SIMD stuff?

@perfinion perfinion self-requested a review September 3, 2018 05:16
@rongjiecomputer
Copy link
Contributor Author

@perfinion I will rebase and update this PR when your PR is merged. It is just that all my Windows PRs tend to drag very long before it gets imported for some reason. Having to keep updating PR due to newer changes breaking my PR is pretty frustrating.

@perfinion
Copy link
Member

@rongjiecomputer yep I hear you. Ping me when it's rebased and I'll help push it through. Thanks for the patience :-)

@rongjiecomputer
Copy link
Contributor Author

Can I get a CI test? Thanks!

@meteorcloudy meteorcloudy added the kokoro:force-run Tests on submitted change label Sep 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 6, 2018
@perfinion
Copy link
Member

@rongjiecomputer Looks like some windows test failures. can you take a look?
Also can you squash those commits together when you update it? git rebase -i origin/master then change the pick to squash. The early commits are just confusing with the new 2.0 stuff so better have it all in one so there are no issues when merging in later.

- Add config/msvc.h when building nasm on Windows
- Update Windows SIMD for libjpeg-turbo 2.0.0
- Add missing source files
@rongjiecomputer
Copy link
Contributor Author

@perfinion Windows builds failed because symbols from simd/x86_64/jsimd.c was missing. Added them back.

I have squashed the commits as requested. I did not do it initially as I thought it will make it difficult for reviewer to see what I have changed since last review.

@perfinion perfinion added the kokoro:force-run Tests on submitted change label Sep 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 6, 2018
@tensorflow-copybara tensorflow-copybara merged commit d41f5ff into tensorflow:master Sep 8, 2018
tensorflow-copybara pushed a commit that referenced this pull request Sep 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants