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

Fix incorrect result of complex log/log10/pow on ARM64 #2870

Merged
merged 4 commits into from
Jul 21, 2022

Conversation

statementreply
Copy link
Contributor

@statementreply statementreply commented Jul 17, 2022

  • Perma-workaround bad codegen of vfma_f64 ARM64 intrinsic, using std::fma instead. This also improves codegen (https://godbolt.org/z/7rd4rc3nY).
  • Use std::fma for clang on ARM64.
  • Drive-by: Fix two lines of comments that are swapped.

Fixes #2857. Fixes DevCom-10088405 and internal VSO-1567425 / AB#1567425 .

I'd appreciate help to run the tests on ARM64 and ARM64EC (especially GH_000935_complex_numerical_accuracy), as I don't have access to an ARM64 Windows machine now.

@statementreply statementreply requested a review from a team as a code owner July 17, 2022 07:20
@statementreply statementreply changed the title Fix incorrect result of complex log on ARM64 Fix incorrect result of complex log/log10/pow on ARM64 Jul 17, 2022
@StephanTLavavej StephanTLavavej added bug Something isn't working ARM64 Related to the ARM64 architecture labels Jul 17, 2022
@strega-nil-ms strega-nil-ms self-assigned this Jul 18, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Without changes:

> python out\arm64\tests\utils\stl-lit\stl-lit.py .\tests\std\tests\GH_000935_complex_numerical_accuracy
Testing Time: 72.23s
  Skipped    : 288
  Unsupported: 216
  Failed     :  72

With change:

Testing Time: 85.56s
  Skipped    : 288
  Unsupported: 216
  Passed     :  72

stl/inc/complex Show resolved Hide resolved
stl/inc/complex Outdated Show resolved Hide resolved
stl/inc/complex Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks! I've pushed minor changes after validating them on x64 (URL update, preprocessor simplification, extra test coverage).

I'm also looking into testing this on an ARM64 VM - I've requested access which should be granted tomorrow.

@strega-nil-ms strega-nil-ms removed their assignment Jul 19, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jul 19, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

(I haven't yet gotten ARM64 VM access, but as the existing behavior is already incorrect for ARM64, I'd rather merge this change before the window for 17.4 Preview 2 closes.)

@StephanTLavavej
Copy link
Member

I have verified that this works on ARM64:

C:\Users\stl\OneDrive - Microsoft\Documents>set PROCESSOR
PROCESSOR_ARCHITECTURE=ARM64
PROCESSOR_IDENTIFIER=ARMv8 (64-bit) Family 8 Model D0C Revision 301, Ampere(R)
PROCESSOR_LEVEL=3340
PROCESSOR_REVISION=0301

C:\Users\stl\OneDrive - Microsoft\Documents>main.exe
Assertion failed: check_result(result, c), file test.cpp, line 98
abort() has been called
C:\Users\stl\OneDrive - Microsoft\Documents>gh_2870.exe

C:\Users\stl\OneDrive - Microsoft\Documents>

@strega-nil-ms
Copy link
Contributor

@StephanTLavavej did you not see the test I did 😝 ?

@StephanTLavavej
Copy link
Member

@strega-nil-ms I totally failed reading comprehension 😹 - didn't see arm64 in the path! Thanks for testing it comprehensively (I tested only one hacked-together configuration manually).

@StephanTLavavej StephanTLavavej merged commit 04ee878 into microsoft:main Jul 21, 2022
@StephanTLavavej
Copy link
Member

Thank you so much for investigating and fixing this runtime correctness bug reported by users!

🐞 ✅ 😻

JMazurkiewicz pushed a commit to JMazurkiewicz/STL that referenced this pull request Jul 26, 2022
@statementreply statementreply deleted the fix-arm64-complex-log branch July 31, 2022 05:47
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARM64 Related to the ARM64 architecture bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GH_000935_complex_numerical_accuracy test fails on ARM64 w/ native compiler
3 participants