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

[Alex Shepherd]

> > 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....  ;-)

[snip...]

> 
> > Likewise here since bit_is_set() is defined by avr-libc thusly:
> >
> >   #define bit_is_set(sfr, bit)   (sfr & _BV(bit))
> >
> > Again, I think replacing '==' with '&&' will correct the code.
> 
> Ditto.  Now, if we are to discuss the _correctness_ of the
> code, that is a different question from the one originally 
> asked. (Can you tell I'm in pedantic mode today?  Jeez, I 
> think I should get out more.... *sigh*)
> 
> > Other than those logical errors, I think Neil's comments ring true.

Guys, in terms of logic, I have already been here and done this and it
doesn't work, hence the awkward looking code with the ==.

What it is testing for is for collisions on a bit-bashed serial network.
So it sets the Tx pin and then a little later checks the Rx pin to see
if it is following the state of the Tx pin. The Tx pin drives the base
of an open collector transistor which inverts the signal, so if the pin
states are the same there has been a collision. So it needs to know if
the both pins are in the same state, be they 0 or 1 or not, hence the ==
and not a &&.

You suggested code only tests that both are on the 1 state which is only
half the time...

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
===============================
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 ) ;
    }
  }
}
===============================
Here is the resulting code from the LST file.
Is there a better way to look at the resluting output as LST files are
hard to follow?
===============================
431                             .type   updateFunctions5to8, @function
 432                    updateFunctions5to8:
 116:../../loconet/throttle.c **** 
 117:../../loconet/throttle.c **** static void updateFunctions5to8(
THROTTLE_DATA_T *ThrottleRec, byte Func5to8, byte ForceNotify )
 118:../../loconet/throttle.c **** {
 433                            .stabn
68,0,118,.LM37-updateFunctions5to8
 434                    .LM37:
 435                    /* prologue: frame size=0 */
 436 012a CF92                  push r12
 437 012c DF92                  push r13
 438 012e EF92                  push r14
 439 0130 FF92                  push r15
 440 0132 0F93                  push r16
 441 0134 1F93                  push r17
 442 0136 CF93                  push r28
 443 0138 DF93                  push r29
 444                    /* prologue end (size=8) */
 445 013a EC01                  movw r28,r24
 446 013c F62E                  mov r15,r22
 447 013e C42E                  mov r12,r20
 119:../../loconet/throttle.c ****   byte Diffs ;
 120:../../loconet/throttle.c ****   byte BitIndex ;
 121:../../loconet/throttle.c ****   byte Mask ;
 122:../../loconet/throttle.c **** 
 123:../../loconet/throttle.c ****   if( ForceNotify ||
ThrottleRec->Func5to8 != Func5to8 )
GAS LISTING C:\DOCUME~1\alexs\LOCALS~1\Temp/ccKSaaaa.s
page 11


 448                            .stabn
68,0,123,.LM38-updateFunctions5to8
 449                    .LM38:
 450                    .LBB5:
 451 0140 4423                  tst r20
 452 0142 19F4                  brne .L33
 453 0144 8A85                  ldd r24,Y+10
 454 0146 8617                  cp r24,r22
 455 0148 01F1                  breq .L31
 456                    .L33:
 124:../../loconet/throttle.c ****   {
 125:../../loconet/throttle.c ****     Diffs = ThrottleRec->Func5to8 ^
Func5to8 ;
 457                            .stabn
68,0,125,.LM39-updateFunctions5to8
 458                    .LM39:
 459 014a EA84                  ldd r14,Y+10
 460 014c EF24                  eor r14,r15
 126:../../loconet/throttle.c ****     ThrottleRec->Func5to8 = Func5to8
;
 461                            .stabn
68,0,126,.LM40-updateFunctions5to8
 462                    .LM40:
 463 014e FA86                  std Y+10,r15
 127:../../loconet/throttle.c **** 
 128:../../loconet/throttle.c ****       // Check Functions 5-8
 129:../../loconet/throttle.c ****     for( BitIndex = 0; BitIndex < 4;
BitIndex++ )
 464                            .stabn
68,0,129,.LM41-updateFunctions5to8
 465                    .LM41:
 466 0150 DD24                  clr r13
 467 0152 00E0                  ldi r16,lo8(0)
 468 0154 10E0                  ldi r17,hi8(0)
 469                    .L40:
 130:../../loconet/throttle.c ****     {
 131:../../loconet/throttle.c ****       Mask = (byte)((byte)1 <<
(byte)BitIndex) ;
 470                            .stabn
68,0,131,.LM42-updateFunctions5to8
 471                    .LM42:
 472 0156 81E0                  ldi r24,lo8(1)
 473 0158 90E0                  ldi r25,hi8(1)
 474 015a 002E                  mov r0,r16
 475 015c 02C0                  rjmp 2f
 476 015e 880F          1:      lsl r24
 477 0160 991F                  rol r25
 478 0162 0A94          2:      dec r0
 479 0164 E2F7                  brpl 1b
 480 0166 482F                  mov r20,r24
 132:../../loconet/throttle.c **** 
 133:../../loconet/throttle.c ****       if( ForceNotify || Diffs & Mask
)
 481                            .stabn
68,0,133,.LM43-updateFunctions5to8
 482                    .LM43:
 483 0168 CC20                  tst r12
 484 016a 19F4                  brne .L39
 485 016c 8E2D                  mov r24,r14
 486 016e 8423                  and r24,r20
 487 0170 31F0                  breq .L36
 488                    .L39:
 134:../../loconet/throttle.c ****         notifyThrottleFunction(
ThrottleRec->UserData, (byte) (BitIndex + 5), Func5to8 & Mask ) ;
 489                            .stabn
68,0,134,.LM44-updateFunctions5to8
 490                    .LM44:
 491 0172 4F21                  and r20,r15
 492 0174 8D2D                  mov r24,r13
 493 0176 8B5F                  subi r24,lo8(-(5))
GAS LISTING C:\DOCUME~1\alexs\LOCALS~1\Temp/ccKSaaaa.s
page 12


 494 0178 682F                  mov r22,r24
 495 017a 8B85                  ldd r24,Y+11
 496 017c 00D0                  rcall notifyThrottleFunction
 497                            .stabn
68,0,129,.LM45-updateFunctions5to8
 498                    .LM45:
 499                    .L36:
 500 017e D394                  inc r13
 501 0180 0F5F                  subi r16,lo8(-(1))
 502 0182 1F4F                  sbci r17,hi8(-(1))
 503 0184 83E0                  ldi r24,lo8(3)
 504 0186 8D15                  cp r24,r13
 505 0188 30F7                  brsh .L40
 135:../../loconet/throttle.c ****     }
 136:../../loconet/throttle.c ****   }
 137:../../loconet/throttle.c **** }
 506                            .stabn
68,0,137,.LM46-updateFunctions5to8
 507                    .LM46:
 508                    .L31:
 509                    .LBE5:
 510                    /* epilogue: frame size=0 */
 511 018a DF91                  pop r29
 512 018c CF91                  pop r28
 513 018e 1F91                  pop r17
 514 0190 0F91                  pop r16
 515 0192 FF90                  pop r15
 516 0194 EF90                  pop r14
 517 0196 DF90                  pop r13
 518 0198 CF90                  pop r12
 519 019a 0895                  ret
 520                    /* epilogue end (size=9) */