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

ARM64 support #131

Closed
1 of 3 tasks
breznak opened this issue Nov 29, 2018 · 6 comments · Fixed by #439
Closed
1 of 3 tasks

ARM64 support #131

breznak opened this issue Nov 29, 2018 · 6 comments · Fixed by #439

Comments

@breznak
Copy link
Member

breznak commented Nov 29, 2018

from numenta#1439

Check if we can build on Arm64 too.
Should be easier, as ASM and platform specific code was mostly removed.

Thanks for testing when you have time, @paulscode

@paulscode
Copy link

For ARM CI, you may be able to leverage Docker by adding the qemu-arm-static binary to the image.

COPY qemu-arm-static /usr/bin

Image will then run natively on ARM and emulate ARM with qemu on x86.

@breznak
Copy link
Member Author

breznak commented Nov 29, 2018

For ARM CI, you may be able to leverage Docker by adding the qemu-arm-static binary to the image.

That'd be cool! And by CI you're talking about Travis? I'll see if we can manage Linux as well as ARM CI under one account (?)

@paulscode
Copy link

I'm not personally familiar with Travis. We use custom pipelines executing on our own servers at my job, so we have a lot more flexibility for customization. Just pointing out that you can run code on other architectures via Docker and qemu, in case it opens up some additional opportunities (easier than having separate hardware for each different architecture).

@breznak
Copy link
Member Author

breznak commented Nov 29, 2018

Thanks, we can run docker on Travis. It'll be really nice to have a separate CI running for this

@breznak breznak added this to the Repo setup milestone Jan 14, 2019
@breznak breznak self-assigned this Jan 14, 2019
@breznak breznak removed their assignment Apr 23, 2019
@breznak
Copy link
Member Author

breznak commented Apr 23, 2019

@brev Moving here discussion from #134 (comment)

[ 63%] Building CXX object src/test/CMakeFiles/unit_tests.dir/unit/ntypes/BasicTypeTest.cpp.o
/usr/local/src/nupic.cpp/src/test/unit/ntypes/BasicTypeTest.cpp:115:49: error: narrowing conversion of '-128' from 'int' to 'nupic::Byte {aka char}' inside { } [-Wnarrowing]
Byte bufByte[8] = {0, 1, 2, 3, 4, 5, -128, 127};

"char" is signed on x86 and x64, and unsigned on ARM. The error refers to the fact that the compiler is being told to convert int 128 to a signed char.

  • a simple fix would be to add a specific cast static_cast<Byte>(-128) which would stay unchanged on x86, and underflow intentionally on arm.

Fixing this type of errors should be relatively easy, as we define and use our types in types/Types.hpp, so we could use #ifdef to change for ARM.

Specifically about byte (from above Types.hpp):

/**
* Represents a signed 8-bit byte.
* (note that std::byte is unsigned char.)
*/
#ifdef Byte
#undef Byte
#endif
typedef char Byte;

@dkeeney we've discussed this, we do we need a signed byte? Ideally cross-platform would be std::byte.

@dkeeney
Copy link

dkeeney commented Apr 24, 2019

do we need a signed byte? Ideally cross-platform would be std::byte.

As far as I know it has always been a signed char. I have no reason for making it signed. But if we change it to unsigned then we may need to add some casts (or maybe remove some) in the places it is used. Personally I would prefer the unsigned char (std::byte) as being a Byte.

@brev brev mentioned this issue Apr 27, 2019
4 tasks
@breznak breznak mentioned this issue May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants