build: enforce SSE2 on x86 targets #2746
Merged
+41
−71
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Absolutely. If this works and the tests pass without the fudge function, we should remove the fudging to restore the sensitivity.The other stuff that was failing in the wallet tests I was able to fix by using the Allocation class and avoiding doubles altogether.Sent from my iPhoneOn Mar 2, 2024, at 10:07 AM, div72 ***@***.***> wrote:See the commit message on the first commit for more details.
@jamescowens I reverted your fixes since I think it's better if we get tests failures in case our assumptions are violated in the future in a similar way, but I can drop the reversions if you want.
You can view, comment on, or merge this pull request online at:
#2746
Commit Summary
f80976d build: enforce SSE2 on x86 targets
feabb82 Revert "Implement comp_double comparison function in certain tests"
4228fed Revert "Cleanup wallet_tests.cpp and remove the rest of double usage"
069d1e0 Revert "Change some tests to use Fraction class in wallet tests"
File Changes (4 files)
M
configure.ac
(10)
M
src/test/gridcoin/claim_tests.cpp
(34)
M
src/test/gridcoin/researcher_tests.cpp
(68)
M
src/test/wallet_tests.cpp
(150)
Patch Links:
https://github.com/gridcoin-community/Gridcoin-Research/pull/2746.patch
https://github.com/gridcoin-community/Gridcoin-Research/pull/2746.diff
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Ah. I realized you reverted all of the commits. I think we should keep the fixes in wallet tests (not the others) as fp should probably never have been used there in the first place? |
This commit targets the floating point errors caused by x87 floating point calculations breaking floating point equality on the testing suite. Let's take the disassembly from `project.m_rac == 123.45` check at src/test/gridcoin/researcher_tests.cpp#L272 for example: ``` │ 0x5911c7c5 <MiningProject::it_falls_back_to_compute_a_missing_external_cpid_invoker()+1989> fldl -0xa4(%ebp) ... │ 0x5911c7e0 <MiningProject::it_falls_back_to_compute_a_missing_external_cpid_invoker()+2016> fldt -0x6fa844(%esi) ... │ 0x5911c81d <MiningProject::it_falls_back_to_compute_a_missing_external_cpid_invoker()+2077> fucompp │ 0x5911c81f <MiningProject::it_falls_back_to_compute_a_missing_external_cpid_invoker()+2079> fnstsw %ax ``` The fldl instruction loads a double to the floating point registers, while the fldt instruction loads a long double(80-bits) to registers. Combining that with the fact that since ASLR is enabled, the one with the massive offset against the stack is probably the 123.45 float literal while the first instruction is for the project.m_rac. Before the fucompp instruction(which compares two floating points), the floating point registers look like this: ``` st0 123.449999999999999997 (raw 0x4005f6e6666666666666) st1 123.450000000000002842 (raw 0x4005f6e6666666666800) ``` The x87 floating point registers seem to work like a stack, since the first fldl instruction loads the 123.450...2842 and the second fldt instruction loads the 123.449...97 value. From the raw value, it seems the first 8 bytes of the values are identical, but the last two bytes (corresponding to the extra 16 bits granted by the x87 extension, which should map to the fraction part of the float) differ slightly, which seems to cause the fucompp instruction to think that these two values are different. This is normally not a problem, as floating point equality comparisons are expected to be not stable. The problem however arises from the fact that it is the **literal 123.45** whose value is off. When a basic C program is compiled(with -m32 option or in a 32-bit environment), the loaded value for 123.45 is identical to the computed value in the st1 register; which means something is going wrong in either during runtime loading of the value or compile-time storing of the value. GDB is unable to actually read that memory address weirdly, so I'm unable to exactly pinpoint which. Because of the issue lying on the extra bits of the fp register, I assumed that the -ffloat-store(which should've ensured registers to not have more precision than a double) or the -fexcess-precision=standard(an option which should be a superset of the former) or the -mpc64(an option which rounds the significand of the results of FP ops to 53-bits) should have fixed the issue, but they didn't and I will not bother to re-examine the disassembly to figure out why. Rather than that, this commit enforces the SSE2 instructions for the FP operations, which are already used on x86_64 hosts and operate under double precision instead of long double. This should be fine compatibility wise, as SSE2 is supported on CPUs after Pentium 4 where I assume the prior CPUs don't have enough computing power to run the client in the first place.
This reverts commit 7bfd1b4.
jamescowens
added a commit
to jamescowens/Gridcoin-Research
that referenced
this pull request
Apr 10, 2024
Added - build: add option for sanitizers gridcoin-community#2553 (@div72) - build: CMake: Initial Windows support (MSYS2) gridcoin-community#2733 (@CyberTailor) Changed - build: enforce SSE2 on x86 targets gridcoin-community#2746 (@div72) - consensus: Update checkpoint data for mainnet and testnet gridcoin-community#2756 (@jamescowens) - gui, util: Enhance verify checkpoints fail handling; use RegistryBookmarks for DB passivation gridcoin-community#2758 (@jamescowens) Removed Fixed - build, depends: fix compilation with XCode 15 gridcoin-community#2747 (@div72) - Fix man page installation path for cmake builds gridcoin-community#2749 (@theMarix) - consensus, mrc, sidestake: add mrc fees to staker to rewards to be allocated via sidestaking gridcoin-community#2753 (@jamescowens) - Fix Systemd unit install location gridcoin-community#2754 (@theMarix) - scraper: Corrections to scraper_net after removal of cntPartsRcvd decrement and increment gridcoin-community#2755 (@jamescowens) - rpc: fix setban segfault gridcoin-community#2757 (@div72)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See the commit message on the first commit for more details.
@jamescowens I reverted your fixes since I think it's better if we get tests failures in case our assumptions are violated in the future in a similar way, but I can drop the reversions if you want.