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

Build and testing for arm64 #714

Merged
merged 4 commits into from
Feb 3, 2021
Merged

Build and testing for arm64 #714

merged 4 commits into from
Feb 3, 2021

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Feb 2, 2021

Closes #710.

@chfast chfast marked this pull request as ready for review February 2, 2021 11:09
@chfast chfast requested review from axic and gumb0 February 2, 2021 11:10
@codecov
Copy link

codecov bot commented Feb 2, 2021

Codecov Report

Merging #714 (1bb6264) into master (e4cc8eb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #714   +/-   ##
=======================================
  Coverage   99.32%   99.32%           
=======================================
  Files          73       73           
  Lines       10885    10885           
=======================================
  Hits        10811    10811           
  Misses         74       74           
Flag Coverage Δ
rust 99.61% <ø> (ø)
spectests 90.84% <ø> (ø)
unittests 99.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@@ -26,6 +26,50 @@ if(TESTFLOAT_GEN)
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
# TODO: Clang produces -0 for input 0, see https://bugs.llvm.org/show_bug.cgi?id=47393.
list(APPEND IGNORE_LIST ui64_to_f64/min)
elseif(CMAKE_CROSSCOMPILING AND CMAKE_SYSTEM_PROCESSOR STREQUAL arm64)
# These tests fails on arm64 emulation. TODO: Check if NaNs are the reason.
Copy link
Member

Choose a reason for hiding this comment

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

@chfast you mentioned these tests are more strict than wasm requires them, but okay on x86. Perhaps mention that here so it is not "misleading" that arm is just failing expected things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I disabled NaN checking for these.

@@ -702,6 +718,9 @@ workflows:
- spectest:
requires:
- coverage-clang
- arm64:
requires:
- coverage-gcc
Copy link
Member

Choose a reason for hiding this comment

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

Why coverage-gcc? That is the only gcc job we have?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Arbitrary ordering. Both use GCC compiler.


string(REPLACE ";" " " EMULATOR "${CMAKE_CROSSCOMPILING_EMULATOR}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why is this replacing needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Explained.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But why can't it be with spaces from the beginning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It must be "a semicolon-separated list, then the first value is the command and remaining values are its arguments."
https://cmake.org/cmake/help/latest/prop_tgt/CROSSCOMPILING_EMULATOR.html

set(CMAKE_SYSTEM_PROCESSOR arm64)
set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_C_COMPILER aarch64-linux-gnu-gcc)
set(CMAKE_CXX_COMPILER aarch64-linux-gnu-g++)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this file be named aarch64-linux perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can be arm64-linux if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is more correct that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the other case, I prefer arm64 over aarch64. It is more informative. Kinda similar issue to x86-64 vs amd64.

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Apart from those two minor comments I think it looks good.

@chfast chfast merged commit 57fdd8f into master Feb 3, 2021
@chfast chfast deleted the arm64 branch February 3, 2021 10:42
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.

Create a CI job for little endian ARM
3 participants