Re: [avr-gcc-list] 8 bit bitwise operation promoted to 16 bit values

[David Brown]

(The indenting in my reply seems to have got mixed up - I hope you can guess
who wrote what...)

> > Won't this always evaluate to false unless (LN_TX_BIT == LN_RX_BIT)?

> YES! That's why I did what I did.

> >
> > The only way I can see this working is like this:
> >
> >      if ( LN_TX_PORT & ( 0x01 << LN_TX_BIT ) &&
> >           LN_RX_PORT & ( 0x01 << LN_RX_BIT ) )
> >      {
> >          ...
> >      }
>
> Indeed this would be better.  I constructed my example as
> close as possible to the original so that the OP would see
> the important difference to get the compiler to generate
> 8-bit code.  Nothing was discussed about the correctness....  ;-)

I don't know why this seems to be so hard for people to get correct.  First,
and most importantly, the code must work - it is easy to write fast, elegant
code that doesn't work but it doesn't really get you anywhere.  It is also
possible to write working code that is big and slow (see the original code).
While gcc is pretty smart at optomisations, it is still constrained by the
limitations of C, including demands to "promote" 8-bit values to integers.
Remember always to make the compiler do the work, not the target processor.
And that means, when possible, that you must try to restrict shifts to
purely constants.  An expression like "if (Port & (1 << bitNo))" reduces to
a single bit-test instruction (or two instructions, for ports outside of the
bit-testing range), so that's what you want to use.  If it is not actually
part of a test, but is used in (say) a comparison, then more code is needed
to get the actual value, giving you larger, slower code.  For example, the
following will work, but will not be optimal in size and speed (note the !
operator, turning all non-zero values to 0):
    if ( !(LN_TX_PORT & ( 0x01 << LN_TX_BIT )) ==
        !(LN_RX_PORT & ( 0x01 << LN_RX_BIT )) ) ...

What is really needed here is an ^^ operator:
    if ( (LN_TX_PORT & ( 0x01 << LN_TX_BIT )) ^^
        (LN_RX_PORT & ( 0x01 << LN_RX_BIT )) ) ...

Of course, that doesn't exist, so you have to write it out manually.  It
could be written neater with macros, but if you only need it once (or can
put it in an inline function, for example), then it doesn't matter that it's
a bit ugly (assuming I've got the brackets right :-) :

    if ( ((LN_TX_PORT & ( 0x01 << LN_TX_BIT )) &&
        (LN_RX_PORT & ( 0x01 << LN_RX_BIT )) ) ||
         (! (LN_TX_PORT & ( 0x01 << LN_TX_BIT )) &&
          ! (LN_RX_PORT & ( 0x01 << LN_RX_BIT )) ) ) ...

That will give you minimal bit-test instruction code.  It might also be
neater, clearer and more useful to split it into a nested if, testing each
bit individually.  Perhaps you can distinguish between 1/0 and 0/1 error
cases ?


[snip...]


> Now back to 8 and 16 bit operations that I was actually asking about...
> :)

> Here is another case where it promotes to 16 bit that is unnecessary
> IMHO.

> The problem in this case is the BitIndex in the for(;;) loop and Mask
> both use 16 bit operations.
> As you can see I have tried everything to tell it that Mask is only a
> byte

It never helps to use (byte) on something that is already a byte.  This is
not quite the same as the earlier case, but again you are suffering from the
C rule "when in doubt, promote to integer", and you are getting it because
you are making the processor do all the work.  When faced with "x << y",
where x and y are not both constants, the compiler has no choice but to use
ints.  That's the way C works, like it or not (I don't).  The compiler
doesn't know that BitIndex is always less than 4 - this would require pretty
advanced code analysis, and anyway you have a function call within the
loop - C cannot assume that the function does not modify the local variable
(because C is a stupid language designed to save keystrokes, not a language
designed for safe usage and optimal code generation).  So the compiler has
to follow the rules and use 16-bit code, even though it will then drop the
high byte.

The answer is, as is the case in almost all optomisation, to re-think your
algorithm.  In this case, you want the inner bit of code to be:
    Mask = 0x01;
    for (BitIndex = 0; BitIndex < 4; BitIndex++) {
        if ( ForceNotify || Diffs & Mask ) ...
        Mask <= 1;
    }


Remember Knuth's two rules of optomisation:
    1) Don't do it.
    2) (For experts only) Don't do it yet.


And when looking at / posting generated code, give us a note of your
optomisation settings, as it can make a big difference.

mvh.,

David




===============================
static void updateFunctions5to8( THROTTLE_DATA_T *ThrottleRec, byte
Func5to8, byte ForceNotify )
{
  byte Diffs ;
  byte BitIndex ;
  byte Mask ;

  if( ForceNotify || ThrottleRec->Func5to8 != Func5to8 )
  {
    Diffs = ThrottleRec->Func5to8 ^ Func5to8 ;
    ThrottleRec->Func5to8 = Func5to8 ;

      // Check Functions 5-8
    for( BitIndex = 0; BitIndex < 4; BitIndex++ )
    {
      Mask = (byte)((byte)1 << (byte)BitIndex) ;

      if( ForceNotify || Diffs & Mask )
        notifyThrottleFunction( ThrottleRec->UserData, (byte) (BitIndex
+ 5), Func5to8 & Mask ) ;
    }
  }
}

<snip assembly output>