Detailed analysis of NaN check stack usage needed

[mlyle]

Relating to #1866 / #1832.

[mlyle]

So @tracernz investigated some: https://github.com/TauLabs/TauLabs/pull/1866#issuecomment-136607919

The sum of it is, that he saw increased stack usage and additional FP instructions across all files that include the misc_math header, like a lot of fast-math is being disabled.

There's a lot of potential approaches that can be taken to fix this.

• First, determine whether the attribute is needed. That is, see whether isfinite() becomes a no-op with -ffast-math. If not, we just use it.
• If it is, then we can do this: implement IS_NOT_FINITE in a separate c file with fast-math disabled. Do not put the attribute in the public header.
• Alternatively, there are approaches we could experiment with-- making it a true macro that only comes into source files that use IS_NOT_FINITE. Implement it as raw ARM assembly that does what we want. etc. Option 2 is better, probably.

[kubark42]

According to --fast-math, isfinite() "should" become a no-op. Although the compiler "shouldn't" add FP instructions randomly, so who knows what is going on. I look at this and wonder if __attributes() is somehow not being bounded to only the IS_NOT_FINITE() function.

• In misc_math.h we can turn off optimization options with #pragma directives, instead of using __attributes. This is another way to arrive at the same results for actually testing NaN. We have stronger guarantees about when the #pragma is turned back off.
• In the Makefile, we can expand --fast-math into its subcomponents, leaving out --finite-math-only and see if things get worse.
• Speaking of macros, we could do a bitwise comparison to look for NaN. Not the cleanest solution, but it will work.

[mlyle]

head with 1866 and 1832 reverted vs. head:
-altitudeHoldTask   192 static
+altitudeHoldTask   208 static
-AttitudeTask   200 static
+AttitudeTask   208 static
-cubic_deadband 0   static
+cubic_deadband 24  static
-expo3  0   static
+expo3  24  static
+IS_NOT_FINITE  8   static
-pathfollowerTask   248 static
+pathfollowerTask   256 static
-pid_apply  16  static
+pid_apply  24  static
-pid_apply_setpoint 24  static
+pid_apply_setpoint 32  static
-stabilizationTask  288 static
+stabilizationTask  312 static
-updateAttitudeComplementary    192 static
+updateAttitudeComplementary    200 static
-vtol_follower_control_loiter   144 static
+vtol_follower_control_loiter   136 static

[mlyle]

So there's two issues here:

1. How to fix IS_NOT_FINITE. I am not so concerned with that. I will have something better in a few minutes.
2. 1866 has been shown to reduce stack usage in practice significantly. Both in my testing and @tracernz's testing and in #1862. HOWEVER it does not reduce stack usage directly as verified by stack stack usage analysis. It was my understanding and belief that we have an IRQ stack and the only thing we should expect to show up on the task stack are the actual functions invoked, but #1866 seems to change usage without changing the actual functions called. #1866 has been confirmed to improve performance. If stack usage is somehow performance sensitive WE NEED TO UNDERSTAND THIS.

[mlyle]

@peabody124 @solidgoldbomb @lilvinz @kubark42 #2 above, any ideas?

[mlyle]

With attribute:

  81:/home/mlyle/TauLabs/flight/Libraries/math/misc_math.h **** static inline bo
ol IS_NOT_FINITE(float x) {
 367                            .loc 6 81 0
 368                            .cfi_startproc
 369                            @ args = 0, pretend = 0, frame = 0
 370                            @ frame_needed = 0, uses_anonymous_args = 0
 371                    .LVL34:
 372 0000 08B5                  push    {r3, lr}
 373                    .LCFI13:
 374                            .cfi_def_cfa_offset 8
 375                            .cfi_offset 3, -8
 376                            .cfi_offset 14, -4
 377                    .LBB17:
  82:/home/mlyle/TauLabs/flight/Libraries/math/misc_math.h ****         return (
!isfinite(x));
 378                            .loc 6 82 0
 379 0002 FFF7FEFF              bl      __fpclassifyf
 380                    .LVL35:
 381                    .LBE17:
...

vs

  77:/home/mlyle/TauLabs/flight/Libraries/math/misc_math.h **** static inline bool IS_NOT_FINITE(float x) {
 358                            .loc 6 77 0
 359                            .cfi_startproc
 360                            @ args = 0, pretend = 0, frame = 0
 361                            @ frame_needed = 0, uses_anonymous_args = 0
 362                    .LVL35:
 363 0000 08B5                  push    {r3, lr}
 364                    .LCFI13:
 365                            .cfi_def_cfa_offset 8
 366                            .cfi_offset 3, -8
 367                            .cfi_offset 14, -4
 368                    .LBB17:
  78:/home/mlyle/TauLabs/flight/Libraries/math/misc_math.h ****         return (!isfinite(x));
 369                            .loc 6 78 0
 370 0002 FFF7FEFF              bl      __fpclassifyf

With the attribute removed only new stack vs baseline is IS_NOT_FINITE, size 8.

[mlyle]

Even so, I'm going to propose a more conservative solution, to avoid issues where a future optimizer might decide to eat the isfinite call.

[mlyle]

No, no I'm actually not. I'm going to just remove the attribute as it's least change and proven correct in current output.

[mlyle]

Merged that. Opened a new issue #1891 to encompass further work.