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.

Sign up for GitHub

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

Jump to bottom

[WIP] Enable MSVC build #238

Closed
wants to merge 4 commits into from
Closed

[WIP] Enable MSVC build #238

wants to merge 4 commits into from

Conversation

peterjc123
Copy link
Contributor

@peterjc123 peterjc123 commented Jan 8, 2020 •
edited

Fixes #150.

Worklist:

  • Fix CMake script
  • Remove wrong annotations of FBGEMM_API
  • Replace VLAs with _aligned_malloc and _aligned_free
  • Write assembly code for Windows (I don't think I'm capable of doing that.)

@peterjc123
Copy link
Contributor Author

Current build errors:

src/FbgemmFP16UKernelsAvx2.cc(443,7): error C2059: syntax error: 'volatile'
src/FbgemmFP16UKernelsAvx2.cc(625,7): error C2059: syntax error: 'volatile'
... and more

Explanation:
The gcc style of assembly is very different from the msvc style one. It needs a complete rewrite. BTW, inline assembly is not supported in an x64 project for msvc, too.

This was referenced Jan 8, 2020
Closed
Closed
dskhudia
dskhudia reviewed Jan 8, 2020
View reviewed changes
src/PackDepthwiseConvMatrixAvx2.cc
@@ -88,7 +89,13 @@ PackedDepthWiseConvMatrix::PackedDepthWiseConvMatrix(
// (12, 8), (12, 9), (12, 10), zero, ..., (15, 8), (15, 9), (15, 10), zero
// (28, 8), (28, 9), (28, 10), zero, ..., (31, 8), (31, 9), (31, 10), zero
for (int k1 = 0; k1 < K; k1 += 32) {
#ifdef _MSC_VER
Copy link
Contributor

Choose a reason for hiding this comment

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

fbgemmAlignedAlloc and fbgemmAlignedFree should take care of it

dskhudia
dskhudia reviewed Jan 8, 2020
View reviewed changes
include/fbgemm/QuantUtils.h
@@ -131,7 +131,7 @@ FBGEMM_API std::int64_t
SaturatingRoundingMulWithShift(std::int32_t a, std::int32_t b, int right_shift);

template <typename T>
FBGEMM_API T Requantize(
T Requantize(
Copy link
Contributor

Choose a reason for hiding this comment

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

These are needed for shared library build on Linux. What problems do they create on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not needed because:

  1. They are the in the header file so they will be visible at the user side.
  2. They have the implementation, not just declarations.

@dskhudia
Copy link
Contributor

dskhudia commented Jan 8, 2020

The gcc style of assembly is very different from the msvc style one. It needs a complete rewrite. BTW, inline assembly is not supported in an x64 project for msvc, too.

The current plan is to not have assembly version for windows. We will add a C/C++ only inefficient version for now. Eventually the inline assembly will go away for gcc as well as MSVC as we move to runtime code generation for fp16 GEMMs.

@ykim362
Copy link
Contributor

ykim362 commented Jan 8, 2020 •
edited

The gcc style of assembly is very different from the msvc style one. It needs a complete rewrite. BTW, inline assembly is not supported in an x64 project for msvc, too.

I have another solution which replaces each asm line to intrinsic APIs line by line.
e.g. https://github.com/marian-nmt/FBGEMM/blob/master/src/codegen_fp16fp32.cc#L284
https://github.com/marian-nmt/FBGEMM/blob/master/src/FbgemmFP16UKernelsAvx2.cc#L125

I know this is not the best solution and jit might be a better solution, eventually. But, this works just fine without losing performance on both Linux and windows before the jit code coming.

@peterjc123
Copy link
Contributor Author

@ykim362 So are you interested in creating a new PR or getting the essential changes in this PR?

@ykim362
Copy link
Contributor

ykim362 commented Jan 9, 2020

@peterjc123 I've been working on AVX512 fp16 kernels. I will submit a PR soon. Thanks for bringing this up.

@dskhudia dskhudia mentioned this pull request Jan 9, 2020
Closed
@dskhudia
Copy link
Contributor

dskhudia commented Jan 9, 2020

@peterjc123 Thanks for your contributions. I have incorporated your changes into the following PRs with minor modifications. These PRs also take care of inline assembly by excluding it for MSVC and redirecting the kernel calls to slower reference implementation.

#240
#241
#242

The PRs take care of the build for the core library. We still need to make sure it runs fine as well.
@ykim362 Thank you for making it work on windows in your fork. I will be picking up some changes from your fork to make sure the runtime is also fine.

@peterjc123
Copy link
Contributor Author

peterjc123 commented Jan 10, 2020 •
edited

@dskhudia Great work! What about introducing a Windows CI with the build-only job that shows the current status of the work towards Windows? If you meet trouble doing it using CircleCI, I can help.

@peterjc123 peterjc123 closed this Jan 10, 2020
@dskhudia
Copy link
Contributor

Thanks. @shz0116 is working on adding a windows build-only job using github Actions. If we face issues with setting it up with github Actions, we will appreciate your help in doing it using CircleCI.

dskhudia added a commit to dskhudia/FBGEMM that referenced this pull request Jan 13, 2020
@dskhudia @facebook-github-bot
Allocate memory instead of using variable length arrays (pytorch#241)
eb57829 
Summary:
Pull Request resolved: pytorch#241

For pytorch#150

Some changes picked from with minor modifications  pytorch#238 . Thanks a lot for your contributions.

Reviewed By: jspark1105

Differential Revision: D19331181

fbshipit-source-id: 08dd3a8ffd65f781e3bda8d241b9eb0221f03669
facebook-github-bot pushed a commit that referenced this pull request Jan 13, 2020
@dskhudia @facebook-github-bot
Allocate memory instead of using variable length arrays (#241)
59e8ed5 
Summary:
Pull Request resolved: #241

For #150

Some changes picked from with minor modifications  #238 . Thanks a lot for your contributions.

Reviewed By: jspark1105

Differential Revision: D19331181

fbshipit-source-id: 130961190c19d369e323ca45dcd953c45d3bf964
jspark1105 pushed a commit that referenced this pull request Mar 21, 2020
@dskhudia @jspark1105
Allocate memory instead of using variable length arrays (#241)
d9633ac 
Summary:
Pull Request resolved: #241

For #150

Some changes picked from with minor modifications  #238 . Thanks a lot for your contributions.

Reviewed By: jspark1105

Differential Revision: D19331181

fbshipit-source-id: 130961190c19d369e323ca45dcd953c45d3bf964
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dskhudia dskhudia dskhudia left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Is this available on windows?
3 participants
@peterjc123 @dskhudia @ykim362