32-bit factorial calculator in x86 NASM assembly

This is how it handles input:

• if negative, terminate program

• if 0-2, display input without calculating

• if greater than or equal to 3, calculate and display result

I have also used some macros from an included file:

• nwln: outputs a newline

• PutStr: displays a label

• GetLInt: receives a 32-bit integer

• PutLInt: displays a 32-bit integer

My questions:

• Is my input validation reducing readability, or is that not important as long as the program proceeds with only the correct input?

• Should I need to check for non-numerical values, or does it not really matter?

• Since the result register (EAX) is 32-bit, should I take into account a possible result that may not fit in 32 bits? If I get a larger result, it won't display properly, and the user may be unaware.

• Is doing a bunch of adds still faster than using mul? I'm also avoiding mul because I still have trouble getting all the bits in the right place.

• Should any of these procedures be organized differently? I'm still not sure how important this is in assembly when most procedures have jumps or branches. In this program, it makes sense to have check_input after get_input and with no jump in between them.

Other than that, I'd like a review on anything you may find.

```
%include "macros.s"

.DATA

input_lbl: DB "Number: ", 0
bad_input_lbl: DB "Number must be positive!", 0
result_lbl: DB "Total: ", 0

input: DD 0
result: DD 0

.CODE
.STARTUP

; EAX - input
; EBX - input (for counter)
get_input:
PutStr input_lbl
GetLInt [input]
mov EAX, [input]
mov EBX, [input]

; EBX - input
check_input:
; terminate if input is negative
cmp EBX, 0
jl report_bad_input

; no need to calculate if below 3
cmp EBX, 3
jge calc_factorial

; input is 0, 1, or 2
jmp display_result

report_bad_input:
PutStr bad_input_lbl
.EXIT

; EAX - result
display_result:
nwln
mov [result], EAX

There are a couple of things that may be of use to you for this code, and some general comments that should be useful for any assembly language programs you write in the future.

Make it modular

The code that implements the factorial routine does a je display_result to exit, but that needlessly ties it to the display routine. In assembly language, it's generally better to use either a ret to return (implying it's written as a subroutine) or to have the routine "fall through" the end. Either is acceptable, but implementing as subroutines is more common.

Prefer subroutines to macros

The macros you've got might, underneath, result in a subroutine call to some code other than that which you've posted, or more likely, it implements directly the code used to perform those functions. There might be a slight performance advantage to replicating the code multiple times via a macro invocation, but more often, it just leads to code bloat. (Although even with this "bloat" it's probably smaller than the corresponding C program!)

Comment fully

If we look at the comment for calc_factorial it currently reads this way:

; EAX - current value
    ; EBX - counter
    ; ECX - current total (for multiplication)
    ; EDX - counter (for multiplication)

This may be accurate, but which are inputs? Which are outputs? What does the routine do? I'd recommend a comment style something more like this:

;****************************************************************************
    ; 
    ; PrintString 
    ; prints the string at DS:DX with length CX to stdout
    ;
    ; INPUT:     ds:dx ==> string, cx = string length
    ; OUTPUT:    CF set on error, AX = error code if carry set
    ; DESTROYED: ax, bx
    ;
    ;****************************************************************************

Use mul rather than add

The mul instruction is definitely going to be faster than the corresponding loop with add instructions. The mul instruction as a 32-bit instruction multiplies EAX by some other register, yielding a result in EDX:EAX meaning that the high part of the result will be in EDX and the low 32 bits of the result will be in EAX.

Structure code to avoid unconditional jumps

If we look at the code, (with comments removed for clarity) we can see that the code does a jmp mult_with_addition but it's jumping over an instruction that cannot be reached! For that reason, you could eliminate both jmp instructions.

calc_factorial:
       dec   EBX
       cmp   EBX, 1
       je    display_result

       mov   ECX, EAX
       mov   EDX, EBX
       jmp   mult_with_addition

       jmp   calc_factorial

    mult_with_addition:
       dec   EDX
       cmp   EDX, 0
       je    calc_factorial

       add   EAX, ECX

       jmp   mult_with_addition

Minimize register moves

As much as practical, try to minimize register moves, preferring instead to have the data already in the register you need for instructions or subroutines which have register-specific needs. For example, because the mul instruction uses EDX and EAX, it makes sense to try to make sure that the number being multiplied is already in that register pair.

Don't fail silently

As with any code in any language, failing silently must be avoided. In the case of a factorial operation, an overflow condition should be checked and passed back to the calling code. If you use the mul instruction, and return a 32-bit result in EAX, checking for overflow is as simple as checking EDX for a zero value.

Look carefully at instruction encodings

In one place in your code, you have the instruction cmp EDX,0 followed by a conditional jump based on the Z flag. In 32-bit mode, that instruction is probably coded as 83FA00 but if you had used or EDX,EDX it is likely to be coded as 09D2 which is one byte shorter. However, neither are really needed because the dec EDX instruction before it (encoded as 4A) already sets the Z flag. There may be no reason to care about one or two bytes one way or the other, but if that's the case, why code in assembly at all? If you're going to do it, do it well.

StackExchange Code Review Q#47692, answer score: 5