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

Improve Windows Compatibility(for lib/TH) #2439

Merged
merged 7 commits into from Sep 17, 2017

Conversation

Projects
None yet
3 participants
@peterjc123
Copy link
Contributor

peterjc123 commented Aug 16, 2017

Win64 support for lib/TH

@fmassa
Copy link
Member

fmassa left a comment

Thanks for the PR!
I did a first pass on it, and had a look on the output of the contribuild. TH compiles, but the compiler outputs some warnings that might be good to fix before merging this PR.
Also, I didn't check the implementation inside THAllocator.c.

{
#if defined(USE_C11_ATOMICS)
return atomic_fetch_add(a, value);
#elif defined(USE_MSC_ATOMICS)
assert(sizeof(int) == sizeof(long));
return _InterlockedExchangeAdd((long*)a, value);
//assert(sizeof(int32_t) == sizeof(long));

This comment has been minimized.

@fmassa

fmassa Aug 16, 2017

Member

we can probably remove this comment

{
#if defined(USE_C11_ATOMICS)
return atomic_compare_exchange_strong(a, &oldvalue, newvalue);
#elif defined(USE_MSC_ATOMICS)
assert(sizeof(int) == sizeof(long));
return (_InterlockedCompareExchange((long*)a, (long)newvalue, (long)oldvalue) == (long)oldvalue);
//assert(sizeof(int32_t) == sizeof(long));

This comment has been minimized.

@fmassa

fmassa Aug 16, 2017

Member

this one as well

IMPLEMENT_THFILE_STORAGE(Char, int8_t)
IMPLEMENT_THFILE_STORAGE(Short, int16_t)
IMPLEMENT_THFILE_STORAGE(Int, int32_t)
IMPLEMENT_THFILE_STORAGE(Long, int32_t)

This comment has been minimized.

@fmassa

fmassa Aug 16, 2017

Member

int64_t

TH_API void THBlas_(copy)(long n, real *x, long incx, real *y, long incy);
TH_API void THBlas_(axpy)(long n, real a, real *x, long incx, real *y, long incy);
TH_API real THBlas_(dot)(long n, real *x, long incx, real *y, long incy);
TH_API void THBlas_(swap)(int64_t n, real *x, int64_t incx, real *y, int64_t incy);

This comment has been minimized.

@fmassa

fmassa Aug 16, 2017

Member

nit: there are two spaces after int64_t everywhere in this file

@@ -922,7 +922,7 @@ THDescBuff THTensor_(desc)(const THTensor *tensor) {
int i;
for(i = 0; i < tensor->nDimension; i++) {
if(n >= L) break;
n += snprintf(str+n, L-n, "%ld", tensor->size[i]);
n += snprintf(str+n, L-n, "%lld", tensor->size[i]);

This comment has been minimized.

@fmassa

fmassa Aug 16, 2017

Member

nit: is this the right format? I'm looking at https://stackoverflow.com/questions/9225567/how-to-print-a-int64-t-type-in-c and I'm not sure it's exactly what you are looking for. (This appears elsewhere in the codebase as well). Also, there are warning in the CI due to those changes, with warnings like

warning: format ‘%lld’ expects argument of type ‘long long int *’, but argument 3 has type ‘int64_t * {aka long int *}’ [-Wformat=]
_q = THTensor_fastGet1d(q, rand_ind);

_mask = THRandom_bernoulli(_generator, _q);
_mask = (int) THRandom_bernoulli(_generator, _q);

This comment has been minimized.

@fmassa

fmassa Aug 16, 2017

Member

_mask is a int64_t, so no need for the casting I think

@@ -556,7 +556,7 @@ static size_t THDiskFile_readString(THFile *self, const char *format, char **str
total += TBRS_BSZ;
p = THRealloc(p, total);
}
if (fgets(p+pos, total-pos, dfself->handle) == NULL) /* eof? */
if (fgets(p+pos, (int) (total-pos), dfself->handle) == NULL) /* eof? */
{
if(pos == 0)

This comment has been minimized.

@fmassa

fmassa Aug 16, 2017

Member

There are warnings in the contribuild down in this file, maybe you miss some castings? Example of warning

/home/travis/build/pytorch/pytorch/torch/lib/TH/THDiskFile.c: In function ‘THDiskFile_new’:
/home/travis/build/pytorch/pytorch/torch/lib/TH/THDiskFile.c:618:5: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     THDiskFile_readChar,
     ^

peterjc123 added some commits Aug 17, 2017

@fmassa

This comment has been minimized.

Copy link
Member

fmassa commented Aug 17, 2017

Thanks for the changes!
TH seems now to compile without any warnings.
The build fails because of the dependency of THPP in TH, so we should probably only merge these PRs together.
@apaszke any better ideas on how we can run the tests with all the changes? The monolithic PR was too hard to review, the separate PRs are easier but running the full test suite is trickier.

@peterjc123

This comment has been minimized.

Copy link
Contributor

peterjc123 commented Aug 17, 2017

@fmassa I tried to make CI builds with windows-full branch. Every time a change is made, it will be merged into this branch. Maybe it's useful.
For Linux/Mac Build Status
For Windows Build status

@fmassa

fmassa approved these changes Aug 18, 2017

Copy link
Member

fmassa left a comment

Thanks a lot for your effort!
I think this is ready for merge. TH builds without warnings.
Let's wait until the other folders are reviewed so that we can merge them all.

#elif defined(TH_REAL_IS_LONG)
TH_TENSOR_APPLY(real, self, *self_data = (long)(THRandom_random(_generator) % (LONG_MAX+1UL)););
TH_TENSOR_APPLY(real, self, *self_data = (int64_t)THRandom_random(_generator););

This comment has been minimized.

@fmassa

fmassa Aug 18, 2017

Member

We don't need the modulo here I suppose?

@soumith soumith added this to Ready for Review in PR Status Aug 23, 2017

peterjc123 added some commits Aug 27, 2017

@soumith soumith merged commit 61813cf into pytorch:master Sep 17, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@soumith

This comment has been minimized.

Copy link
Member

soumith commented Sep 17, 2017

Thanks a ton @peterjc123 for doing this big PR. This is really good stuff.

@soumith soumith removed this from Ready for Review in PR Status Sep 25, 2017

@peterjc123 peterjc123 deleted the peterjc123:windows-TH branch Sep 27, 2017

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