HiveBrain v1.2.0
Get Started
← Back to all entries
patternModerate

GCD/LCM calculator in x86 NASM assembly

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
lcmx86assemblygcdcalculatornasm

Problem

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 "> 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, 1

Solution

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

Code Snippets

foo           (1,2);
x = bar       (2.5);
xor       EAX, EAX
xor EAX, EAX
b = a * 5 + 2298;
c = b / 9;
kelvin_x9 = fahrenheit * 5 + 2298;
kelvin = kelvin_x9 / 9;

Context

StackExchange Code Review Q#24288, answer score: 12

Revisions (0)

No revisions yet.