-
Notifications
You must be signed in to change notification settings - Fork 167
Add support for WIN ARM64 #1092
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1092 +/- ##
==========================================
- Coverage 83.23% 83.10% -0.13%
==========================================
Files 56 56
Lines 5808 5766 -42
==========================================
- Hits 4834 4792 -42
Misses 974 974 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it, given Donlan's approval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, this looks correct... but I'm not an expert in the arm64 memory model. How have we tested this for correctness?
@@ -71,6 +89,32 @@ typedef long aws_atomic_impl_int_t; | |||
typedef long long aws_atomic_impl_int_t; | |||
#endif | |||
|
|||
#ifdef _M_ARM64 | |||
/* Hardware Read Write barrier, prevents all memory operations to cross the barrier in both directions */ | |||
# define AWS_RW_BARRIER() __dmb(_ARM64_BARRIER_SY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are new (seemingly) public symbols, which are only defined on MSVC... should we namespace them to make it clear they are internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am undefining them at the bottom of the file, so they should be OK in term of external symbol visibility
* Volatile ops may or may not imply this barrier, depending on the /volatile: switch, but adding an extra | ||
* barrier doesn't hurt. | ||
*/ | ||
# define AWS_SW_BARRIER() _ReadWriteBarrier(); /* software barrier */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a #define?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed, but i thought it looks better
but i can remove it and replace it
@bdonlan, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. While we're doing arm architecture work. i have a branch of aws-c-common that adds feature detection on windows for arm cpu features such as crc and pmull. Should we add that here?
Add support for ARM64 on Windows
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.