Skip to content

Conversation

elikosan
Copy link
Contributor

  • fix vector/avx Windows compile (missing _dllexport + AVX not defined on Windows)
  • fix BLAS functions to operate on 64 bits __int64 (equiv. to long on linux) when compiling 64bit platform
  • fix FindSSE.cmake so it does not crash on Windows (unitialized variable)

@@ -2,22 +2,23 @@
#define TH_AVX_H

#include <stddef.h>
#include "THGeneral.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

AVX.h is included after THGeneral.h in file ./lib/TH/THVector.c. So there is no need to include THGeneral.h here.

void THFloatVector_muls_AVX(float *y, const float *x, const float c, const ptrdiff_t n);
void THFloatVector_cadd_AVX(float *z, const float *x, const float *y, const float c, const ptrdiff_t n);
void THFloatVector_adds_AVX(float *y, const float *x, const float c, const ptrdiff_t n);
TH_API void THDoubleVector_copy_AVX(double *y, const double *x, const ptrdiff_t n);
Copy link
Contributor

Choose a reason for hiding this comment

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

These apis are not supposed to be exported, but are used for specific implementations for different instructions internally. Please refer to lib/TH/generic/THVectorDispatch.c

@@ -1,4 +1,3 @@
#if defined(__AVX__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Though it is harmless to remove the macro check for msvc, I think it can be kept because I am not sure the behavior of other platforms. Besides, this macro is the source I found that caused the compilation broken. #991

@@ -1,4 +1,3 @@
#if defined(__AVX2__)
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as AVX.c

@@ -2,8 +2,9 @@
#define TH_AVX2_H

#include <stddef.h>
#include "THGeneral.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as AVX.h

@elikosan
Copy link
Contributor Author

elikosan commented Apr 3, 2017

I agree with BTNC reviews. Pull request #991 fixes the issues that i was having, thus i reverted the changes to avx.h,avx.c,avx2.h,avx2.c.

@soumith soumith merged commit ca19337 into torch:master Apr 3, 2017
@soumith
Copy link
Member

soumith commented Apr 3, 2017

thanks Eric!

#ifdef WIN32
typedef __int64 BLINT;
#else
typedef long BLINT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this typedef is correct? It used to give sdot_ an int* on non WIN32 platforms, now it uses long*, but sizeof(long) != sizeof(int) on most x86_64 systems. The same problem seems to be present in other parts of this PR too.

Copy link
Contributor

Choose a reason for hiding this comment

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

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