Simple C code inefficient and working

Go To Last Post
12 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have the simplest code to average two 8bit signed numbers, but it is not working, temp1 and temp3 are declared as uin8_t:

temp1=(uint8_t)((temp1-0x80)>>1)+(((uint8_t)temp3-0x80)>>1);
temp1=temp1-0x80;

which translates to:

369:temp1=(uint8_t)((temp1-0x80)>>1)+(((uint8_t)temp3-0x80)>>1);
MOV       R24,R7         Copy register
LDI       R25,0x00       Load immediate
SUBI      R24,0x80       Subtract immediate
SBCI      R25,0x00       Subtract immediate with carry
ASR       R25            Arithmetic shift right
ROR       R24            Rotate right through carry
LDI       R27,0x80       Load immediate
MOV       R7,R27         Copy register
ADD       R7,R24         Add without carry
370:temp1=temp1-0x80;
MOVW      R30,R12        Copy register pair
LDD       R24,Z+0        Load indirect with displacement
LDI       R25,0x00       Load immediate
SUBI      R24,0x80       Subtract immediate
SBCI      R25,0x00       Subtract immediate with carry
ASR       R25            Arithmetic shift right
ROR       R24            Rotate right through carry
ADD       R7,R24         Add without carry

But what I really want the compiler to do is this:

subi R24, 0x80  ; Transform [0x80, 0x7f] -> [0x00, 0xff]
lsr  R24        ; Divide by 2
subi R22, 0x80  ; Transform [0x80, 0x7f] -> [0x00, 0xff]
lsr  r22        ; Divide by 2
add  R24, R22   ; Add
subi R24, 0x80  ; Transform [0x00, 0xff] -> [0x80, 0x7f]

Besides generating inefficient code, the C code also gives bad results with certain values, like adding -1 and 0.
About the averaging method itself, I know I am discarding the LSB from both operands, but this is not important.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
like adding -1 and 0
How can you have an number like -1 if both temp1 and temp3 are defined as unsigned integers?

Regards,
Steve A.

The Board helps those that help themselves.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Koshchi wrote:
Quote:
like adding -1 and 0
How can you have an number like -1 if both temp1 and temp3 are defined as unsigned integers?
The temp variables get their values from signed variables, but internally, the values are still represented the same: -1 = 0xFF, 0=0x00

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

But the math on them will NOT be the same. For the example you gave of -1, (-1 - 0x80) >> 1 will give a result of 0x3F (63). If temp1 was a signed number, the result would be 0xBF (-65). The compiler is obligated to extend the values to 16 bit before doing the math, so 0xFF becomes 0xFFFF when signed and 0x00FF when unsigned.

Regards,
Steve A.

The Board helps those that help themselves.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Post a complete compilable example exhibiting the problem, together with the command line used to compile it and avr-gcc version.

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

The compiler is obligated to extend the values to 16 bit before doing the math,

Perhaps that could be sidestepped?

uint8_t frog;
...
frog = 0x80;

temp1 -= frog;
temp1 >>= 1;
temp1 += (temp3 - frog)>>1;

temp1 -= frog;
...

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Koshchi wrote:
The compiler is obligated to extend the values to 16 bit before doing the math.
But wouldn't this be taken care off by using explicit casts? (The very thing I am trying to do).
And also, why would the second operation be also extended to 16bit?
temp1=temp1-0x80;
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

But wouldn't this be taken care off by using explicit casts? (The very thing I am trying to do).
And also, why would the second operation be also extended to 16bit?

I'm pretty sure it is the constant that is extended to "int" by rule.

Thus my suggestion to have a dummy variable with the 8-bit constant. I'd speculate that the compiler will never even actually create a "frog" address, but that may depend on the rest of the code around the sequence.

Lee

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
(uint8_t)((temp1-0x80)>>1)

The only cast you are doing here is AFTER the math. And as Lee pointed out, 0x80 is signed, so you would need to explicitly cast that. But if these are signed values, then why on Earth are you using unsigned variables to hold them? And what is the purpose of subtracting 0x80 in the first place?

Quote:
And also, why would the second operation be also extended to 16bit?
Because that is what the C standard says must happen.

Regards,
Steve A.

The Board helps those that help themselves.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for helping me find the solution!
By moving the cast around I got the correct result:

temp1=((uint8_t)(temp1-0x80)>>1)+((uint8_t)(temp3-0x80)>>1);
temp1=temp1-0x80;

But I just realized that the best way to achieve the desired result is this:

temp1=((int8_t)(temp1)>>1)+((int8_t)(temp3)>>1); 

Which translates directly into two arithmetic shifts, and an addition. (I was under the wrong impression that a shift was always a logical shift).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
But I just realized that the best way to achieve the desired result is this:
But again, wouldn't it be so much easier to use signed ints in the first place?

Regards,
Steve A.

The Board helps those that help themselves.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Koshchi wrote:
But again, wouldn't it be so much easier to use signed ints in the first place?
Yes, perhaps I should clean up my code and declare and use them temp variables locally, and with the right type (I used those temp variables across my code for simple calculations).