GCD/LCM calculator in x86 NASM assembly After creating a similar program in C++, I've decided to try to write it in x86 assembly language (the type of assembly I was taught in college).

I'd like to receive feedback regarding syntax and clarity.  The macros used here were provided by my college, so I don't think that's a concern.

%include "macros.s"

.DATA

num_lbl:   DB   "&gt; Numbers (2): ", 0
gcd_lbl:   DB   "*** GCD: ", 0
lcm_lbl:   DB   "*** LCM: ", 0

num1:   DD   0
num2:   DD   0

num1_cpy:   DD   0
num2_cpy:   DD   0

gcd:   DD   0
lcm:   DD   0

.CODE
.STARTUP

xor       EAX, EAX
xor       EBX, EBX
xor       ECX, ECX
xor       EDX, EDX
xor       EDI, EDI
xor       ESI, ESI

main:
     nwln
     nwln
     PutStr   num_lbl
     nwln
     nwln
     GetLInt   [num1]
     GetLInt   [num2]

     mov   EAX, [num1]
     mov   [num1_cpy], EAX
     mov   EBX, [num2]
     mov   [num2_cpy], EBX

     call   calc_euclidean
     call   calc_lcm

     nwln
     PutStr   gcd_lbl
     PutLInt   [gcd]
     nwln
     PutStr   lcm_lbl
     PutLInt   [lcm]
     nwln

     .EXIT

calc_euclidean:
               mov   EAX, [num2]
               cmp   EAX, 0
               jne   chk_swap

               mov   EAX, [num1]
               mov   [gcd], EAX

               ret

calc_lcm:
         mov   EAX, [num1_cpy]
         mov   EDX, [num2_cpy]
         mul   EDX

         mov   EDI, EAX

         xor   EBX, EBX

         mov   EDX, EDI
         shr   EDX, 16
         mov   EAX, EDI
         mov   BX, [gcd]
         div   BX

         mov   SI, AX
         mov   [lcm], SI

         ret         

chk_swap:
         mov   EAX, [num1]
         mov   EBX, [num2]
         cmp   EAX, EBX
         jl    swap

after_check:
            jmp   loop

swap:
     mov   EAX, [num1]
     mov   EBX, [num2]

     ; temp
     mov   ECX, [num2]

     ; num2 = num1
     ; num1 = temp
     mov   EBX, EAX
     mov   EAX, ECX

     mov   [num1], EAX
     mov   [num2], EBX

     jmp   after_check

loop:
     mov   EDX, [num1]
     shr   EDX, 16
     mov   EAX, [num1]
     mov   BX, [num2]
     div   BX

     mov   EDI, [num1]
     mov   ESI, [num2]
     mov   EDI, ESI
     mov   [num1], EDI
     mov   [num2], EDX

     jmp   calc_euclidean
1) In a language like C or C++, you wouldn't do something like this:

foo           (1,2);
x = bar       (2.5);


It makes it harder to read the code because the function name makes no sense without its parameters. The same is true for assembly. Basically, this:

xor       EAX, EAX


Should be this:

xor EAX, EAX




2) In a language like C or C++, you wouldn't do something like this:

b = a * 5 + 2298;
c = b / 9;


Instead, you might do this:

kelvin_x9 = fahrenheit * 5 + 2298;
kelvin = kelvin_x9 / 9;


In assembly, because you can't give registers descriptive names (in the same way that you can give variables descriptive names in higher level languages) and because often you do things that are semantically wrong (e.g. using shifts when you mean multiplication, using LEA when you're not loading an effective address, etc) it's extremely important for code maintainability to use comments. Basically this:

calc_lcm:
     mov   EAX, [num1_cpy]
     mov   EDX, [num2_cpy]
     mul   EDX

     mov   EDI, EAX

     xor   EBX, EBX

     mov   EDX, EDI
     shr   EDX, 16
     mov   EAX, EDI
     mov   BX, [gcd]
     div   BX

     mov   SI, AX
     mov   [lcm], SI

     ret


Should be this:

calc_lcm:
     mov EAX, [num1_cpy]   ;eax = number1
     mov EDX, [num2_cpy]   ;edx = number2
     mul EDX               ;edx:eax = number1 * number2

     mov EDI, EAX          ;edi = number1 * number2, high 32-bits of number discarded

     xor EBX, EBX

     mov EDX, EDI          ;edx = number1 * number2
     shr EDX, 16           ;edx = number1 * number2 / 65536, dx:ax = number1 * number2
     mov EAX, EDI          ;eax = number1 * number2, (can be deleted as EAX already contained this value)
     mov BX, [gcd]         ;bx = GCD
     div BX                ;ax = number1 * number2 / GCD, dx = number1 * number2 % GCD

     mov SI, AX            ;si = number1 * number2 / GCD
     mov [lcm], SI
     ret


For assembly there's only 2 types of bugs - comments that don't describe sane logic, and code that doesn't do what the comments say it should. This allows you to debug the code effectively and drastically reduces the chance of missing mistakes (in addition to the hopefully obvious massive improvement in code readability).



3) In a language like C or C++, you would create lots of bugs caused by things like integer overflows because the language doesn't provide a sane/easy way to detect these bugs. In assembly you should not create lots of bugs caused by things like integer overflows because you can easily test for overflows.

For example, this:

calc_lcm:
     mov EAX, [num1_cpy]   ;eax = number1
     mov EDX, [num2_cpy]   ;edx = number2
     mul EDX               ;edx:eax = number1 * number2

     mov EDI, EAX          ;edi = number1 * number2, high 32-bits of number discarded


Should be this:

calc_lcm:
     mov EAX, [num1_cpy]   ;eax = number1
     mov EDX, [num2_cpy]   ;edx = number2
     mul EDX               ;edx:eax = number1 * number2

     test EDX, EDX         ;Will the result fit in 32 bits?
     jne .overflow         ; no, error
     mov EDI, EAX          ;edi = number1 * number2


Note: A better idea would be to do "movzx ebx,word [GCD]" and "div ebx" to divide EDX:EAX by the GCD instead of pointlessly converting the 64-bit value you had in EDX:EAX into a 32-bit value in DX:AX.



4) A compiler will check if (e.g.) functions are being called with the correct number of parameters, and will make sure that various registers are saved/restored, etc where necessary. In assembly there is none of that; so you have to do it manually, so you have to be able to do it manually. To be able to do it manually you need to document routines properly. For example, this:

calc_lcm:


Should be this:

;Calculate Lowest Common Multiple
;
;Input:
; [num1_cpy]  Some number
; [num2_cpy]  Some other number
; [GCD]       The GCD
;
;Output:
; [lcm]       The result
;
;Trashed:
; eax, edx, ebx, esi, edi

calc_lcm:


Normally, you'd pass parameters in registers and return parameters in registers to avoid wasting time on loading/storing where avoidable. For example, the entire "calc_lcm" code might be:

;Calculate Lowest Common Multiple
;
;Input:
; eax         Some number
; edx         Some other number
; ebx         The GCD
;
;Output:
; eax         The result
;
;Trashed:
; edx

calc_lcm:
     mul EDX               ;edx:eax = number1 * number2
     div EBX               ;eax = (number1 * number2) / GCD, edx = (number1 * number2) % GCD
     ret




5) If you're writing assembly you should write assembly; and not attempt to obfuscate the code by inventing your own language that makes it difficult read the code and impossible to effectively optimise the code.

For example, this code:

 nwln
 nwln
 PutStr   num_lbl
 nwln
 nwln
 GetLInt   [num1]
 GetLInt   [num2]


Is 100% meaningless gibberish.

I don't know what this code should be (and am completely unable to see ways of optimising it that would have been obvious if it was assembly) because I have no idea what any of these macros do. I can't tell which registers get trashed, I can't see which pushes/pops/moves are redundant. I might be able to assume that it'd be safe to delete the first 2 nwln lines and change the string to num_lbl:   DB   "\n\n&gt; Numbers (2): ", 0 but then that could introduce all sorts of bugs because I didn't bother with the hassle of finding and reading the macros that may do more (or less) than whatever I've assumed they might do.