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 potentially incorrect floating-point arithmetic on MinGW #50426

Merged
merged 2 commits into from
Aug 5, 2021

Conversation

BrettDong
Copy link
Member

@BrettDong BrettDong commented Aug 2, 2021

Summary

Bugfixes "Fix potentially incorrect floating-point arithmetic on MinGW"

Purpose of change

The following three tests are failing on MinGW cross-compile test workflow:

  • stamina burn for movement
  • player_morale_decay
  • throwing_skill_impact_test

The symptom is that floating point arithmetic in these three failing tests is producing different numeric results to that on other platforms.

Describe the solution

  • player_morale_decay and throwing_skill_impact_test succeeds if I reset x87 FPU and SSE states at the beginning of main function. This seems to be a MinGW bug where floating point math package states are not properly initialised on a Windows thread: https://blog.zaita.com/mingw64-compiler-bug/
  • stamina burn for movement failure is solved by adding std::round() to round a double number to int in burden_player, without which on MinGW 45000 * 1.01 => 45449.

Describe alternatives you've considered

I've tried to call standard library function std::fesetround( FE_TONEAREST ) to set floating point rounding mode to round to nearest representable value, but this does not actually solve the problem, the tests were still failing.

Compiling with -msse -mfpmath=sse -march=pentium4 mentioned by lemire in his blog https://lemire.me/blog/2020/06/26/gcc-not-nearest/ might help, but I haven't tried yet.

Testing

Expect to see MinGW cross-compile test workflow to pass.

Additional context

@BrettDong BrettDong added Code: Tests Measurement, self-control, statistics, balancing. OS: Windows Issues related to Windows operating system labels Aug 2, 2021
@BrettDong BrettDong force-pushed the burn_rate branch 3 times, most recently from 447b9a8 to bb8db75 Compare August 2, 2021 15:58
@BrettDong
Copy link
Member Author

Expected output for the first two assertions:


Capacity: 45000000mg | Burden Proportion: 1.01 | Units: 45450
Burn Ratio (1): 15
Burn Ratio (2): 16
Stamina Multiplier: 1
Burn Ratio (3): 16
Stamina Move Cost Modifier: 1
Stamina:  10000 -> 9984

../tests/char_stamina_test.cpp:295: FAILED:
  CHECK( burdened_burn_rate( dummy, move_mode_walk, 1.01 ) == normal_burn_rate + 10 )
with expansion:
  16 == 16

Capacity: 45000000mg | Burden Proportion: 1.02 | Units: 45900
Burn Ratio (1): 15
Burn Ratio (2): 17
Stamina Multiplier: 1
Burn Ratio (3): 17
Stamina Move Cost Modifier: 1
Stamina:  10000 -> 9983
../tests/char_stamina_test.cpp:296: FAILED:
  CHECK( burdened_burn_rate( dummy, move_mode_walk, 1.02 ) == normal_burn_rate + 20 )
with expansion:
  17 == 17

@actual-nh
Copy link
Contributor

Different rounding (again!)?

@BrettDong
Copy link
Member Author

Yes it seems to be a rounding problem:

Capacity: 45000000mg | Burden Proportion: 1.01 | Units: 45449

45000 * 1.01 should be 45450 but was 45449 in MinGW workflow .

@BrettDong
Copy link
Member Author

BrettDong commented Aug 3, 2021

Setting rounding mode to nearest representable value doesn't help.

std::fesetround( FE_TONEAREST );

@BrettDong
Copy link
Member Author

Stamina test is fixed by std::round. Next to fix is the failing morale decay test.

@BrettDong
Copy link
Member Author

Now throwing test is failing:

 -------------------------------------------------------------------------------
 throwing_skill_impact_test
   hi_skill_basestats_rock
 -------------------------------------------------------------------------------
 ../tests/throwing_test.cpp:224
 ...............................................................................
 
 ../tests/throwing_test.cpp:144: FAILED:
   CHECK( dmg_thresh_met )
 with expansion:
   false
 with messages:
   Monster: 'mon_zombie' Item: 'rock
   Range: 5 Pstats: STR: 8 DEX: 8 PER: 8 SKL: 10
   Total throws: 10000
   Ratio: 100%
   Hit Lower: 99.9235% Hit Upper: 100%
   Hit Thresh: 90% - 110%
   Adj Wald error: 0.000764809
   Avg total damage: 12.8064
   Dmg Lower: 12.7758 Dmg Upper: 12.837
   Dmg Thresh: 13 - 23
   Margin of error: 0.000764809

Expected numbers should be:

  Monster: 'mon_zombie' Item: 'rock
  Range: 5 Pstats: STR: 8 DEX: 8 PER: 8 SKL: 10
  Total throws: 100
  Ratio: 100%
  Hit Lower: 93.2609% Hit Upper: 100%
  Hit Thresh: 90% - 110%
  Adj Wald error: 0.0673911
  Avg total damage: 13.99
  Dmg Lower: 13.6262 Dmg Upper: 14.3538
  Dmg Thresh: 13 - 23
  Margin of error: 0.0673911

The plain static_cast<int>() gives a different rounding result on MinGW
cross compile test workflow on GitHub Actions.
@BrettDong BrettDong force-pushed the burn_rate branch 5 times, most recently from 02e6bd9 to 5833666 Compare August 4, 2021 14:21
@BrettDong BrettDong added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` labels Aug 4, 2021
@BrettDong BrettDong changed the title Debug failing stamina burn test Fix potentially incorrect floating-point arithmetic on MinGW Aug 4, 2021
@@ -91,7 +91,7 @@ static int actual_burn_rate( Character &dummy, const move_mode_id &move_mode )
static void burden_player( Character &dummy, float burden_proportion )
{
units::mass capacity = dummy.weight_capacity();
int units = static_cast<int>( capacity * burden_proportion / 1_gram );
int units = static_cast<int>( std::round( capacity * burden_proportion / 1_gram ) );
Copy link
Member

Choose a reason for hiding this comment

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

I'm very concerned that this leaves the door open for other errors to persist in the codebase that aren't calling std::round(), this behavior needs to be the same in both builds (though if this is an incremental step to getting things unblocked disregard).

Copy link
Member Author

Choose a reason for hiding this comment

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

Even with fpreset this test is still failing, so I'm adding rounding at the moment.

@actual-nh actual-nh mentioned this pull request Aug 5, 2021
4 tasks
@kevingranade kevingranade marked this pull request as ready for review August 5, 2021 07:14
@kevingranade kevingranade merged commit 88ebf77 into CleverRaven:master Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. OS: Windows Issues related to Windows operating system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants