32-bit factorial calculator in x86 NASM assembly

Question

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:

1. Is my input validation reducing readability, or is that not important as long as the program proceeds with only the correct input?
2. Should I need to check for non-numerical values, or does it not really matter?
3. 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.
4. 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.
5. 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
   PutStr    result_lbl
   PutLInt   [result]
   .EXIT

; EAX - current value
; EBX - counter
; ECX - current total (for multiplication)
; EDX - counter (for multiplication)
calc_factorial:
   ; no need to multiply by 1
   dec   EBX
   cmp   EBX, 1
   je    display_result

   ; perform addition instead of multiplication
   mov   ECX, EAX
   mov   EDX, EBX
   jmp   mult_with_addition

   jmp   calc_factorial

; EAX - current total
; ECX - current total (for multiplication)
; EDX - counter (for multiplication)
mult_with_addition:
   ; determine if addition should stop
   dec   EDX
   cmp   EDX, 0
   je    calc_factorial

   add   EAX, ECX

   jmp   mult_with_addition

Sample tests:

Input: 3
Output: 6

Input: 2
Output: 2

Input: 1
Output: 1

Input: 0
Output: 0

Input: -1
Output: "Number must be positive!"

Answer 1

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.

Answer 2

It appears that I've forgotten that 0! is equal to 1, not 0...

I've made the following changes to fix that:

• Change result's starting value from 0 to 1. In this way, 1 will be displayed as a result right away (no calculations done for such input).

• Assigned EAX to result in calc_factorial instead of display_result (it doesn't make sense to mutate values in displaying code anyway...).

  Remove from here:

  display_result:
     nwln
     mov       [result], EAX // remove
     PutStr    result_lbl
     PutLInt   [result]
     .EXIT
  

  Add to here:

  calc_factorial:
     ; no need to multiply by 1
     dec   EBX
     cmp   EBX, 1
     mov   [result], EAX // add
     je    display_result
  

Answer 3

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

Usually, "does it implement required functionality?" is even more important than "is it readable?"

A "Hello world" program is very readable but doesn't implement required functionality.

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

When you're programming for other people (e.g. professionally) there might be someone other than the programmer (e.g. there's a boss, a product manager, a UI designer) who specifies what is implemented: and your job as programmer is to decide how to implement that.

Whether validating input "matters" depends on why you're writing the program, on who you're writing it for, and on who is going to use it.

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.

Again that's a matter of the program's "requirements". Different programs have different requirements.

IIRC a 32-bit number is big enough for 13! but not 14!.

Is doing a bunch of adds still faster than using mul?

This suggests that on a Pentium, ADD is about 3 cycles whereas MUL is about 10 cycles. So MUL is faster if you need to multiply by more than about 3 or 4.

I'm also avoiding mul because getting the bits in the right place for it is complicated, and I still cannot seem to get it right.

Perhaps ask about this on Stack Overflow.

Should any of these procedures be organized differently? I'm still not sure how important this is in assembly when 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.

I suppose there are three reasons to use subroutines:

• Programmers who use high-level-languages are used to them; a nest of jump are "spaghetti code" and "goto is considered harmful" (compared with 'structured programming')
• A subroutine can be called from more than one place (i.e. reused)
• One of the problems with assembly is knowing what registers are used. You documented which registers are used by your code fragments. For larger programs you might want to define a standard used by all subroutine, for example, "any subroutine may use/corrupt eax through edx; if it alters any other register (e.g. esi, ebp, etc.) then it must save previous value before changing it and restore old value after changing it before returning.

For example your program might look like this if it were written with subroutines:

; assume the following calling conventions:
; esi is the input parameter
; eax is the output value
; carry flag is set if there's an error
mov esi, input_lbl
call PutStr
call GetUint
jnc input_ok
mov esi, input_error
call PutStr
.EXIT
input_ok:
mov esi, eax
call factorial
jnc output_ok
mov esi, factorial_overflow_error
call PutStr
.EXIT
output_ok
push eax ; preserve the factorial value
mov esi, factorial_overflow_is
call PutStr
pop esi ; restore the factorial value
call PutUint
.EXIT

---

GetLInt   [input]
mov       EAX, [input]

If GetLInt is a macro would it work to just say GetLInt EAX?

---

mov       EBX, [input]

mov EBX, EAX (i.e. move from register) would be faster and shorter than move from memory.

---

"Number must be positive!", 0

EBX contains bits. Those bits can represent a signed or an unsigned number. You might like to use a different macro e.g. GetLUInt which errors if the user enters a - minus sign, and accepts unsigned integers e.g. 4000000000.

---

nwln

Is this a macro not an opcode? It might be better to use e.g. upper case for macro names and lower case for opcode names.