-
Notifications
You must be signed in to change notification settings - Fork 114
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
Universal abi used in amm functions, no need for OS differentiation #1869
base: main
Are you sure you want to change the base?
Conversation
Hey Dan, thanks for looking into this! Could you please re-enable the code path (https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/cpucap/internal.h#L144) to test the change in the CI? |
2e8d424 does include that change! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1869 +/- ##
=======================================
Coverage 78.53% 78.53%
=======================================
Files 585 585
Lines 99783 99783
Branches 14283 14283
=======================================
+ Hits 78362 78368 +6
+ Misses 20785 20782 -3
+ Partials 636 633 -3 ☔ View full report in Codecov by Sentry. |
### Description of changes: This extends the work done on #1869. * Updates to latest version of Intel SDE; * Fixes minor bug in Go test script. * Disables use of AVX512 IFMA on Windows. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --------- Co-authored-by: Dan Pittman <dan@dpitt.me> Co-authored-by: Justin Smith <justsmth@amazon.com>
During the review process, I mistakenly believed that the
@_6_arg_universal_ABI
did not account for Windows, as it conspicuously matches the Linux calling convention. Additionally, Windows does not have 6 registers in its calling convention, only 4—args 5 and 6 would be pushed onto the stack, so what I had here was wrong in two ways.While debugging, I could see that this was segfaulting during a read of a pointer in
%r11
; this was what tipped me off.Running
crypto_test
was successful locally on:On an SPR-based system. I have not tested this change on Linux locally yet, but I will in the next few hours.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.