Repeated function calls in statement - Bad practice?

[dannyf]

My reasoning is that some odd compiler might optimize out the repeated calls and use the same value?

It is always possible that some buggy compilers may do that but that's not an issue with the code.

Having said that, I tend to write out the statement one at a time to make it more readable to myself.

Code: [Select]

   if(s3 == 0) {
      /* First time we have read the battery voltage */
     /* Assuming function never returns 0 */
      s0 = MeasureBattADC();
      s1 = MeasureBattADC();
      s2 = MeasureBattADC();
   } else {
      /* Move samples along */
      s0 = s1;
      s1 = s2;
      s2 = s3;
   }

Much easier to initialize s0..2 so you don't have to do the test each time you call that routine.

Code: [Select]

  curr_avg = ((ALPHA * sample) +
        curr_avg -
        ((ALPHA * curr_avg) / DENOM));

    return (curr_avg / DENOM);
Not an efficient / good implementation - there have been discussions about that in the past here.

[djacobow]

Code: [Select]

  curr_avg = ((ALPHA * sample) +
        curr_avg -
        ((ALPHA * curr_avg) / DENOM));

    return (curr_avg / DENOM);

Not an efficient / good implementation - there have been discussions about that in the past here.

Please elaborate or point, as I'm always willing to learn. This implementation results in no multiplies or divides. I know it suffers from from a loss of range due to the the state being shifted by DENOM's bits.

What else is wrong?

[dannyf]

when alpha*curr_avg is smaller than denom, ....

instead, keep track of the sum (ie. upscale everything by denom).

[djacobow]

when alpha*curr_avg is smaller than denom, ....

instead, keep track of the sum (ie. upscale everything by denom).

Look more closely at the code. Everything is upscaled by denom, and in fact, it works just fine with small samples.

I'm not saying this code doesn't have other problems, but it doesn't have that particular problem. I admit it's a bit tricky to see because of the way I named the variables.

Here's the same thing run, but with the input samples limited to between 0 and 15, so much smaller than 64 in this case:

[dannyf]

This is what I use:

Code: [Select]

//calculate exponentially smoothed moving average
int ma0(int sample) {
static int sum=0, avg=0; //sum and avg

sum+=(sample-avg); //update sum
avg=sum/MA_WGT; //calculate the moving average
return avg;
}

MA_WGT is the same as your DENOM. ALPHA is set to 1.

It should be much faster than yours. And you may be able to prove mathematically that they are the same.

[djacobow]

This is what I use:

Code: [Select]

//calculate exponentially smoothed moving average
int ma0(int sample) {
static int sum=0, avg=0; //sum and avg

sum+=(sample-avg); //update sum
avg=sum/MA_WGT; //calculate the moving average
return avg;
}

MA_WGT is the same as your DENOM. ALPHA is set to 1.

It should be much faster than yours. And you may be able to prove mathematically that they are the same.

First, yes, mathematically they are the same. And in fact, if you pass in ALPHA=1 the compiler correctly eliminates it entirely.

However, yours probably is /not/ faster -- not that it matters on something so minimalist.

The salient difference between implementations is that you store the sum and avg and I only store the sum. Storing the sum saves you a calculation on each iteration (a shift), but it costs you an extra load and store.

I put your code into a class and compiled it with gcc-arm -Os -fno-inline -S to see what comes and they were just as you'd expect:

Mine:

Code: [Select]

.fnstart
.LFB15:
ldr r3, [r0]
lsr r2, r3, #6
sub r3, r3, r2
add r1, r3, r1
str r1, [r0]
@ sp needed
lsr r0, r1, #6
bx lr

Yours:

Code: [Select]

dannyf_ema_c<long, long, 1ul, 64ul>::update(long):
.fnstart
.LFB16:
ldr r3, [r0]
ldr r2, [r0, #4]
sub r3, r3, r2
add r1, r3, r1
str r1, [r0]
lsr r1, r1, #6
str r1, [r0, #4]
@ sp needed
mov r0, r1
bx lr


If you allow inlining, I suspect yours might do a bit better because the compiler might be able to leave sum/avg in registers.

[wolf32d]

Quick question.

Do you think this is bad practice?

Code: [Select]

uint32 BattADC_AVG = ( (MeasureBattADC() + MeasureBattADC() + MeasureBattADC() + MeasureBattADC() ) >> 2);
My reasoning is that some odd compiler might optimize out the repeated calls and use the same value?
Or should that never happen?

Since you evaluate a sum which is commutative everything is fine.
BUT if you perform some non-commutative operation involving nasty function calls (like in the following code), unexpected things can (and will) happen

Code: [Select]

#include <stdio.h>

// global var
float a;


float func()
{
    a += 1.0;
    return a;
}


int main()
{
    a = 0;
    printf("%.1f\n",  func() - func() ); //expression 1
   
    a = 0;   
    printf("%.1f\n", -func() + func() ); //expression 2
   
}
The first printf gives -1 as expected:
the first func() call gives 1 and the second gives 2 therefore the result is -1.

At this point one might think that the second expression should give (-1 + 2 = +1) but it gives -1 instead. The compiler (gcc) optimizes the second expression and produces the same machine code for both expressions.

[Boomerang]

I'm curious about the following - is there compiler that will optimize this line

y = sin(x) + sin(x) + sin(x) + sin(x);

to call sin() function only once?

[obiwanjacobi]

How would the compiler know it is a function (in math sense) and not a method (changing state)? 

[wolf32d]

How would the compiler know it is a function (in math sense) and not a method (changing state)? 

A good compiler can distinguish math functions and functions that modify some data or some global vars. Gcc can perform some optimizations on math expressions. For example x / y can be replaced with x * (1/y) which is useful if (1/y) is subject to common subexpression elimination.

[Boomerang]

OK, the question was poorly defined...

My question is: is there any compiler that could be instructed that some function is deterministic (does not modify anything outside of it's scope and for a given set of parameters always returns the same result)?

[grumpydoc]

OK, the question was poorly defined...

My question is: is there any compiler that could be instructed that some function is deterministic (does not modify anything outside of it's scope and for a given set of parameters always returns the same result)?

Yes, marking a function "pure" in gcc means that it only uses its arguments and global variables and is a candidate for common subexpression elimination. Marking a function "const" means it only uses its arguments and does not read global memory (ie "const" is a stricter form of "pure").

[andyturk]

And in C++ (since C++11), there's a constexpr keyword that tells the compiler a function will return a constant result (when passed a constant argument). The main use of constexpr is to write functions that can be evaluated at compile-time.

[boffin]

I'm curious about the following - is there compiler that will optimize this line

y = sin(x) + sin(x) + sin(x) + sin(x);

to call sin() function only once?

Yes, me

Code: [Select]

y = 4 * sin(x)