search

dlaidig / vqf

137 stars 24 forks source link

Issues with -ffinite-math-only compiler flags #18

Closed tw0b33rs closed 1 week ago

tw0b33rs commented 3 months ago

Hi Daniel,

first, very useful library you have provided here.

I noticed a problem (compiler related) and managed to nail it down to the updateAcc() method in combination with the -ffinite-math-only compiler flag. This is part of the -ffast-math compiler flag that we usually use. For some reason, it causes the quaternion to change the order of the elements/signs in each iteration.

Example (quaternion w; x; y; z): Iteration 1: 0.000005260;1.000000000;0.000000732;-0.000009787 Iteration 2: -1.000000000;-0.000007799;0.000001655;-0.000015989 Iteration 3: 0.000019527;-0.999999999;0.000032044;-0.000005745 Iteration 4: -0.000042049;0.999999997;-0.000063022;0.000020478 Iteration 5: 0.999999998;0.000031121;0.000013145;0.000047899 Iteration 6: -0.999999995;-0.000052445;-0.000028611;-0.000077813

If I don't use the -ffinite-math-only compiler flag, then everything behaves as it should. The flag "Allow optimizations for floating-point arithmetic that assume that arguments and results are not NaNs or +-Infs.". It seems that the updateAcc() method does not take this into account. I don't think this is a general limitation. However, a note might be useful for future users of the library.

Best regards, Tobias

dlaidig commented 3 months ago

Hi Tobias,

According to the first (very useful) article I found when googling for -ffast-math, isnan() will always return false with this flag.

VQF uses NaN as a sentinel value in various places. It should be easy to modify the code to use a different value instead (e.g., std::numeric_limits<vqf_real_t>::max()). All you would have to do is to search the code for NaN and isnan.

If there are no other issues introduced by -ffast-math, this should work. In general, after skimming over the article I linked above, I would argue that -ffast-math should probably only be used with code written with this compiler flag in mind and not with third-party code.

tw0b33rs commented 3 months ago

Hi Daniel,

thanks for your evaluation and suggestion. I agree, the -ffast-math compiler argument needs to be used carefully. This issue can be closed for now.

Best, Tobias