Converting decimal to binary in Assembly

Question

The longest example program in my PicoBlaze Simulator in JavaScript is this decimal-to-binary converter:

;This is an example program that uses
;UART, the interface that PicoBlaze uses
;for connecting to terminals (a DOS-like
;user interface, with a keyboard and a
;screen capable of displaying text).
;It loads base-10 integer numbers from
;the terminal, converts them into binary,
;and then prints the binary
;representations back onto the terminal.
;Example input would be: 

;1
;2
;4
;8
;15
;127
;255
;

;And the expected output is: 

;1_(10)=1_(2)
;2_(10)=10_(2)
;4_(10)=100_(2)
;8_(10)=1000_(2)
;15_(10)=1111_(2)
;127_(10)=1111111_(2)
;255_(10)=11111111_(2)
;

;Note that you need to click the
;"Enable UART" button in order to use it.
;Also, the trailing empty line in the
;input is necessary for the result to be
;printed.

;Now follows some boilerplate code
;we use in our Computer Architecture
;classes...
CONSTANT LED_PORT,00
CONSTANT HEX1_PORT,01
CONSTANT HEX2_PORT,02
CONSTANT UART_TX_PORT,03
CONSTANT UART_RESET_PORT,04
CONSTANT SW_PORT,00
CONSTANT BTN_PORT,01
CONSTANT UART_STATUS_PORT,02
CONSTANT UART_RX_PORT,03
; Tx data_present
CONSTANT U_TX_D, 00000001'b
; Tx FIFO half_full
CONSTANT U_TX_H, 00000010'b
; TxFIFO full
CONSTANT U_TX_F, 00000100'b
; Rxdata_present
CONSTANT U_RX_D, 00001000'b
; RxFIFO half_full
CONSTANT U_RX_H, 00010000'b
; RxFIFO full
CONSTANT U_RX_F, 00100000'b

ADDRESS 000
START:
;At the beginning, the number is 0.
load s0,0
;And we are storing its string
;representation at the beginning
;of RAM.
namereg s3,pointer
load pointer,0
;Now follows a loop to load
;the digits of the number.
loading_the_number:
  ;Load a character from the UART
  ;terminal.
  call UART_RX
  ;Check whether the character is a digit.
    compare s9,"0"
    ;If it is not a digit, jump to the
    ;part of the program for printing
    ;the number you have got.
    jump c,print_the_number
    load s1,"9"
    compare s1,s9
    jump c,print_the_number
  ;If it is a digit, store it into RAM.
  store s9,(pointer)
  add pointer,1
  ;Multiply the number you have got by 10.
  load sf,s0
  call multiply_by_10
  load s0,se
  ;Then, convert the digit from ASCII
  ;into binary.
  sub s9,"0"
  ;And then add it to the number you
  ;have got.
  add s0,s9
  call c,abort ;In case of overflow.
  jump loading_the_number ;Repeat until a
                          ;non-digit is
                          ;loaded.
print_the_number:
;If there are no digits to be printed,
;do not print anything.
sub pointer,0
jump z,START
print_the_decimal:
load s4,pointer
load pointer,0
printing_the_decimal_loop:
  compare pointer,s4
  jump nc, end_of_printing_the_decimal
  fetch s9,(pointer)
  ;Do some basic sanity check: Is the
  ;character you are printing indeed
  ;a decimal digit?
    compare s9,"0"
    call c,abort
    load s1,"9"
    compare s1,s9
    call c,abort
  ;If it is indeed a decimal digit,
  ;print it.
  call UART_TX
  add pointer,1
  jump printing_the_decimal_loop
end_of_printing_the_decimal:
;After you have repeated the decimal
;number, print the string "_(10)=".
load s9,"_"
call UART_TX
load s9,"("
call UART_TX
load s9,"1"
call UART_TX
load s9,"0"
call UART_TX
load s9,")"
call UART_TX
load s9,"="
call UART_TX
;If the number to be printed is
;equal to zero, print 0.
sub s0,0
jump nz,print_the_binary
load s9,"0"
call UART_TX
jump end_of_printing_loop
print_the_binary:
;Make the pointer point to the
;beginning of RAM.
load pointer,0
;Now goes a loop which stores the binary
;representation of the number we have
;got into RAM, but reversed.
beginning_of_converting_to_binary:
  sub s0,0
  jump z,end_of_converting_to_binary
  load s9,"0"
  sr0 s0
  jump nc,store_digit_to_memory
  add s9,1
  store_digit_to_memory:
  store s9,(pointer)
  add pointer,1
  jump beginning_of_converting_to_binary
end_of_converting_to_binary:
;Do some basic sanity check, such as that
;the pointer does not point to zero.
compare pointer,0
call z,abort ;Something went wrong
             ;so end the program.
;Check whether there are more than 8 bits.
compare pointer,9
call nc,abort
;Now goes a loop which will print
;the binary number in RAM, with digits
;in the correct order. The pointer now
;points at a memory location right after
;the binary number (not at the last digit,
;but after it).
beginning_of_printing_loop:
  sub pointer,1
  jump c,end_of_printing_loop
  fetch s9,(pointer)
  ;Do some basic sanity check:
  ;Is the character the pointer points to
  ;indeed a binary digit?
    compare s9,"0"
    jump z,memory_is_fine
    compare s9,"1"
    jump z,memory_is_fine
    call abort ;Something went wrong,
               ;so end the program.
  memory_is_fine:
  ;If everything is fine, print that
  ;digit.
  call UART_TX
  ;Repeat until you have printed all
  ;digits of the binary number
  ;stored in RAM.
  jump beginning_of_printing_loop
end_of_printing_loop:
;After you have printed that binary
;number, print the string "_(2)" and
;a new-line.
load s9,"_"
call UART_TX
load s9,"("
call UART_TX
load s9,"2"
call UART_TX
load s9,")"
call UART_TX
load s9,a ;newline character, 0xa=='\n'.
call UART_TX
;The program runs in an infinite loop...
JUMP START

multiply_by_10:
  load se,sf
  add se,se
  call c,abort
  add se,se
  call c,abort
  add se,sf
  call c,abort
  add se,se
  call c,abort
return

abort:
  load s9,"E"
  call UART_TX
  load s9,"R"
  call UART_TX
  load s9,"R"
  call UART_TX
  load s9,"O"
  call UART_TX
  load s9,"R"
  call UART_TX
  load s9,"!"
  call UART_TX
  load s9,a ;newline
  call UART_TX
  infinite_loop:
  jump infinite_loop
return

;Now follows some boilerplate code
;we use in our Computer Architecture
;classes...
UART_RX:
  INPUT sA, UART_STATUS_PORT
  TEST sA, U_RX_D
  JUMP Z, UART_RX
  INPUT s9, UART_RX_PORT
RETURN

UART_TX:
  INPUT sA, UART_STATUS_PORT
  TEST sA, U_TX_F
  JUMP NZ, UART_TX
  OUTPUT s9, UART_TX_PORT
RETURN

So, what do you think about it? Do you have some suggestions about how to make it better?

Answer 1

Readability is poor

You should insert more empty lines in order to cleanly separate the logical code blocks.

Your use of indentation is not all that consistent. It will improve readability a lot if you get this right.

Also, seeing that you can use some alias (namereg s3,pointer), I think you could use this more often to improve readability.

I feel your 'basic sanity checks' are more of 'insanity' checks. You should not doubt that much. The extra code also increases the risk of errors.

When something is wrong, you go to the abort part of the program never to return. It's confusing to see that most of the time you use a call to do this and not the much clearer jump.

---

Optimizations

If you're using the ASCII character set, then the colon character (":") follows the character "9". You can simplify the verification of a valid digit (no longer needing that temporary register s1) like:

compare s9,"0"
jump c,print_the_number
compare s9,":"
jump nc,print_the_number

Minimizing the number of instructions, and certainly control transferring instructions, is important. I have moved the more likely jumping back above the unlikely jumping to abort.

jump nc,loading_the_number ;Repeat until a non-digit is loaded.
jump abort

;If the number to be printed is
;equal to zero, print 0.
sub s0,0
jump nz,print_the_binary
load s9,"0"
call UART_TX
jump end_of_printing_loop

You don't need to special-case if the number happens to be zero. It will shave off numerous instructions.

Most of your loops are WHILE-WEND loops that have a condition on top and an unconditional jump that goes back to the top. Using REPEAT-UNTIL loops will be faster and require but one conditional jump instruction.

The new code:

START:
  load s0,0
  namereg s3,pointer
  load pointer,0

loading_the_number:
  call UART_RX
  compare s9,"0"
  jump c,print_the_number
  compare s9,":"
  jump nc,print_the_number
  store s9,(pointer)
  add pointer,1
  load sf,s0
  call multiply_by_10
  load s0,se
  sub s9,"0"
  add s0,s9
  jump nc,loading_the_number ;Repeat until a non-digit is loaded.
  jump abort

print_the_number:
  sub pointer,0 ;If there are no digits to be printed, do not print anything.
  jump z,START

  load s4,pointer
  load pointer,0
printing_the_decimal_loop:
  fetch s9,(pointer)
  call UART_TX
  add pointer,1
  compare pointer,s4
  jump c,printing_the_decimal_loop

  ... some fixed text gets outputted here


  load pointer,0                    ; Works even if s0 == 0
beginning_of_converting_to_binary:  ; This stores in reverse order!
  load s9,"0"
  sr0 s0
  jump nc,store_digit_to_memory
  load s9,"1"
store_digit_to_memory:
  store s9,(pointer)
  add pointer,1
  sub s0,0
  jump nz,beginning_of_converting_to_binary

  ; pointer is certainly not zero
  sub pointer,1
beginning_of_printing_loop:
  fetch s9,(pointer)
  call UART_TX
  sub pointer,1
  jump nc,beginning_of_printing_loop

  ... some fixed text gets outputted here

  JUMP START