Compiler generating incorrect code for integer math

[Bridgerman]

Hi Folks,

My first post so go easy on me!

I have something very simple that I cannot seem to get to work.

I have two integer variables: Descend_start, and Delay_time and all I want to do is subtract them.

For the purpose of this example, Descend_start = 0x0123 and Delay_time = 0x0086.
0x0123 - 0x0086 should yield 0x009D but the results I get are 0xFF9D

I am using a PIC16F688 device and here is the code that is generated:

;delay_time = descend_start - delay_time
movf DELAY_TIME,W
subwf DESCEND_START,W
movwf DELAY_TIME
movf DESCEND_START_H,W
movwf DELAY_TIME_H
btfss STATUS,C
incfsz DELAY_TIME_H,W
subwf DELAY_TIME_H,F

I'll duplicate the code here and describe the problem between steps. Step numbers added for clarity
Minuend (Descend_start) = 0x0123 and Subtrahend (Delay_time) = 0x0086
1) movf DELAY_TIME,W
All good here, setting up W with the LS byte of the subtahend
2) subwf DESCEND_START,W
All good here, subtracting the LS byte of the subtrahend from the minuend and carry = 0 indicating a borrow
3) movwf DELAY_TIME
All good here, storing the LS byte of the results (0x9D) in the proper destination register
4) movf DESCEND_START_H,W
Loads MS minuend (Descend_start_H) into W register
5) movwf DELAY_TIME_H
Stores MS minuend byte into the results
6) btfss STATUS,C
Checks to see if there was a borrow, and there was so step 7 is executed
7) incfsz DELAY_TIME_H,W
Increments the MS byte of the minuend and stores results in W, result is not zero (2) executes step 8
8) subwf DELAY_TIME_H,F
Subtracts the (MS minuend + 1) from Delay_time_H which holds the MS minuend from step 5

The result of the subtraction always yields 0xFF since subtracting the minuend+1 from the minuend results in -1

Is this a known bug? How can simple two's complement math not work?

Thank you,
bridgerman

[Anobium]

Welcome,

Just use the power of the compiler.

Dim myResult as Word, Descend_start, Descend_time

 myResult  = Descend_start - Delay_time

[Bridgerman]

Thanks Anobium,

I am using the compiler. And the numbers are integers.

The source code is
Dim Descend_start as Integer
Dim Delay_time as Integer
Delay_time = Descend_start - Delay_time

When looking at the assembly code that GCB generated I get:
;delay_time = descend_start - delay_time
movf DELAY_TIME,W
subwf DESCEND_START,W
movwf DELAY_TIME
movf DESCEND_START_H,W
movwf DELAY_TIME_H
btfss STATUS,C
incfsz DELAY_TIME_H,W
subwf DELAY_TIME_H,F

The code generated doesn't seem to be correct since the answer is wrong.

John

[Anobium]

Odd issue, I think I have seen this before where a constant showed the same sort of issue. The compiler seem to handle the assignment same variable as the second variable in the calculation incorrectly.

So, to workaround multiple by 1 - this causes the use of the temp variable which is then assigned correctly .

#chip 16f688, 4
#option explicit

Dim Descend_start as Integer
Dim Delay_time as Integer
Dim Delay_result as Integer


#DEFINE USART_BAUD_RATE 9600
#DEFINE USART_TX_BLOCKING
#DEFINE USART_DELAY OFF

Descend_start   = 0x0123  '291
Delay_time      = 0x0086  '134

' Calc is
' 291-134 = 157


Delay_time = (Descend_start - Delay_time)*1
HSerPrint Delay_time

Do this workaround work?

[Bridgerman]

Interestingly enough, that does work. It is still a compiler bug though. I though I would post here so it could be corrected at some point in time. I ported the assembly code output to MPLAB so I could run a simulation. Without walking through the assembly code with MPLAB running a single-step simulation and viewing all the registers, I would never have found this. The equation is embedded in a much longer string of equations and the bottom line answer was wrong.

To work around it, I just used a temporary variable
Dim Descend_start as Integer
Dim Delay_time as Integer
Dim Temp as Integer
Temp = Descend_start - Delay_time
Delay_time = Temp

[Bridgerman]

If the subtraction was:
Dim Descend_start as Integer
Dim Delay_time as Integer
Descend_start = Descend_start - Delay_time

The compiled code is correct:
;Descend_start = Descend_start - Delay_time
movf DELAY_TIME,W
subwf DESCEND_START,F
movf DELAY_TIME_H,W
btfss STATUS,C
incfsz DELAY_TIME_H,W
subwf DESCEND_START_H,F

Why would the destination difference produce incorrect code?

Using an intermediate variable, the compiler also generates correct code:
Dim Descend_start as Integer
Dim Delay_time as Integer
Dim Temp as Integer
Temp = Descend_start - Delay_time

;temp = descend_start - delay_time
movf DELAY_TIME,W
subwf DESCEND_START,W
movwf TEMP
movf DESCEND_START_H,W
movwf TEMP_H
movf DELAY_TIME_H,W
btfss STATUS,C
incfsz DELAY_TIME_H,W
subwf TEMP_H,F

[Anobium]

Agree this is a bug. Compiler needs to do a proper job or at least complain/warn user.

I now know the strategy to force the compiler to handle this simple calc but I need to determine if the scope is just integer or all types.

[Anobium]

Resolved in build 1094. Published today.

It was quite complex to resolve.

The changed was essentially to ensure these calculations use a temp variable. You would have thought that was simple but there are many use cases that use the same piece the compiler. So, I have isolated to simple maths of variable types but not Bytes (as bytes did not exhibit the issue), ensure the target variable was part of the calculation (lots of issues do this simple task) and the instructing the compiler to use a temp variable.

Should this cause any issue the code (new change) can be turned off by defining a constant called DISABLE1094

Hope this resolves forever.

[Bridgerman]

Thank you for the quick fix, Anobium!

[Anobium]

Can you let know if it resolves. Thanks.

[Bridgerman]

Anobium,

Can you point me to some directions as to how to load the patch? I downloaded it but I'm not sure what to do next.

Thank you

[Anobium]

See here https://sourceforge.net/projects/gcbasic/files/Release%20Candidates/Patches/

This has a zip and instructions

[Anobium]

I have have run all the static tests and I find the results very interesting.

1. No errors reported - which is great
2. But, clearly the error you spotted has not caused issues before across 3000 test programs. But, when I looked most used temp vars, or a variant of the calculation approach that did not raise the issue. Amazing really.
3. The new approach does not increase program size in the majority of cases as the calculation was assessed by the compiler as complex (which implies the use of a temp var).

So, things look good from my end.

[Bertrand BAROTH]

Did this bug affect AVR code, too, on build 1094 ?

[Anobium]

Great question. The bug WAS PIC only. However, I only test on PIC... and, #1094 has broken AVR maths on one of the eight cases tested. So, I needed to isolate the change further to constrain to resolve the PIC issue only.

So, get build 1096 to ensure you have the corrected compiler.

[Anobium]

Something was bugging me .. 'how was this issue never spotted before' and 'why did is not affect 100s of other programs'?

After a lot more investigation this issue only affected ChipFamily 14. So, on specific set of chips.

So, et build 1097 to ensure you have the corrected compiler as this build only changes the ASM for the ChipFamily 14 chips.

[Sleepwalker3]

How do you tell if a particular chip is "ChipFamily 14"?

Was Hugh able to offer any further info?

[Anobium]

The issue is resolved.

How do you tell if a particular chip is "ChipFamily 14"?

Look at the ChipData tab using PICINFO.

Was Hugh able to offer any further info?

No.

[Anobium]

This is still bugging me. I have email Hugh to see if he can advise.

The issue is shown in the code below. The code below represents the code generates.

;
; 16 bit unsigned subtraction with carry out.
; Word format is little endian (LSB at lower address)
; Operation is DST = DST - SRC
;
;
; DST is replaced, SRC is preserved, Carry is set correctly
;
;

        MOVF    (SRC),W         ; Get low byte of subtrahend
        SUBWF   (DST),F         ; Subtract DST(low) - SRC(low)
        MOVF    (SRC)+1,W       ; Now get high byte of subtrahend
        BTFSS   STATUS,C        ; If there was a borrow, rather than
        INCF    (SRC)+1,W       ; decrement high byte of dst we inc src
        SUBWF   (DST)+1,F       ; Subtract the high byte and we're done

Note the DST is replaced. And, SRC is preserved.

However, the issue is we need the SRC to be replaced. As in this operation is SCR = DST - SRC. I have searched the web to find example ASM but not able to find. The fix I have applied does resolve the issue but I want to understand how this simple calc would look in ASM.

[SPostma]

This is an easy fix, but with 2 instructions extra:

    ; sameroutine  as above, but result stored in (SRC)
    MOVF    (SRC),W         ; Get low byte of subtrahend
    SUBWF   (DST),W         ; Subtract DST(low) - SRC(low), --> W
    MOVWF   (SRC)           ; store W result in (SRC)

    MOVF    (SRC)+1,W       ; Now get high byte of subtrahend
    BTFSS   STATUS,C        ; If there was a borrow, rather than
    INCF    (SRC)+1,W       ; decrement high byte of dst we inc src
    SUBWF   (DST)+1,W       ; Subtract the high byte, result in W
    MOVWF   (SRC)           ; store W result in (SRC)

[Jerry Messina]

That last " MOVWF (SRC) ; store W result in (SRC)" is a little suspect... (SRC)+1?

[SPostma]

yes you are correct, well spoted Jerry!

[Anobium]

Thanks for the posts. I need someone to validate the change I have already made. For safety and to ensure I did not break anything else (as recursion testing is very hard without all the use cases) I have used a temp variable. This may not be optimised but safety is the most important factor.

Dest = (Src - Dest)

MOVF    DEST,W
SUBWF   SRC,W
MOVWF   SYSTEMP1
MOVF    SRC_H,W
MOVWF   SYSTEMP1_H
MOVF    DEST_H,W
BTFSS   STATUS,C
INCFSZ  DEST_H,W
SUBWF   SYSTEMP1_H,F
MOVF    SYSTEMP1,W
MOVWF   DEST
MOVF    SYSTEMP1_H,W
MOVWF   DEST_H