50
\$\begingroup\$

Since this problem involves small numbers (particularly with a small loop count of 100), it's possible to ease the modulo operation setup by simply working with 16-bit and 8-bit registers:

$$\dfrac{\text{[AX] (16-bit register)}}{\text{[other 8-bit register]}} = \text{[AH] (remainder)}$$

My main concern is with the layout. Every "basic" high-level implementation I've seen has a check and print together for each case. I've found it easier to do the same thing here, but I'm not sure if that would also be too readable in assembly.

I'm also aware that it's good to minimize register-moving. Unfortunately, I still do that with every case since I'm incrementing with one register (CX) and using another (AX) for the dividend. I can stick to AX for both, but that may involve keeping a copy of the current counter value, which may just make the code a bit more complicated. I suppose it's not much of a problem here anyway.

Macros used:

  • nwln - prints a newline
  • PutStr - prints a defined string
  • PutInt - prints a 16-bit integer value

It's not necessary to address the macros; they do work properly.

%include "macros.s" .DATA fizz_lbl: DB "Fizz", 0 buzz_lbl: DB "Buzz", 0 fizzbuzz_lbl: DB "FizzBuzz", 0 .CODE .STARTUP xor CX, CX ; counter main_loop: inc CX cmp CX, 100 jg done fizzbuzz_check: mov AX, CX ; dividend = counter mov BH, 15 ; divisor div BH ; (counter / 15) cmp AH, 0 ; counter divisible by 15? je print_fizzbuzz ; if so, proceed with printing jmp fizz_check ; if not, try checking for fizz print_fizzbuzz: PutStr fizzbuzz_lbl nwln jmp main_loop fizz_check: mov AX, CX ; dividend = counter mov BH, 3 ; divisor div BH ; (counter / 3) cmp AH, 0 ; counter divisible by 3? je print_fizz ; if so, proceed with printing jmp buzz_check ; if not, try checking for buzz print_fizz: PutStr fizz_lbl nwln jmp main_loop buzz_check: mov AX, CX ; dividend = counter mov BH, 5 ; divisor div BH ; (counter / 5) cmp AH, 0 ; counter divisible by 5? je print_buzz ; if so, proceed with printing jmp print_other ; if not, then can only display number print_buzz: PutStr buzz_lbl nwln jmp main_loop print_other: PutInt CX nwln jmp main_loop done: .EXIT
\$\endgroup\$
7
  • 1
    \$\begingroup\$ Nice! +1 for the effort writing this is a low level language. May I suggest posting the result set? \$\endgroup\$ Commented Jul 12, 2014 at 22:59
  • 1
    \$\begingroup\$ @Phrancis: For verification of successful execution, you mean? \$\endgroup\$ Commented Jul 12, 2014 at 23:02
  • \$\begingroup\$ Yeah basically. Though I don't doubt it did exec. \$\endgroup\$ Commented Jul 12, 2014 at 23:18
  • 1
    \$\begingroup\$ @Phrancis: I only hesitate because it'll take up a lot (possibly unnecessary) room. Would a link to a screenshot suffice? \$\endgroup\$ Commented Jul 12, 2014 at 23:21
  • 1
    \$\begingroup\$ Where can we obtain macros.s? \$\endgroup\$ Commented Jul 13, 2014 at 2:24

2 Answers 2

Reset to default
37
\$\begingroup\$

Since we're doing this in assembly language, it makes sense to do it much more efficiently than is typically done in high level languages. Otherwise, why bother with assembly language? So with that said, there are ways that this can be made much, much more efficient.

Avoid division

The div instruction in x86 is one of the slower instructions possible. Since we already know that we're looking for numbers divisible by 3, 5 or both, what would make far more sense is to simple keep countdown counters for both. Your initialization currently says:

 xor cx, cx

It could be easily expanded to say:

 xor cx, cx mov bx, 0503h ; set bh = 5 counter, bl = 3 counter

Then instead of dividing, simply decrement:

 inc cx cmp cx, 100 jg done ; instead of this... ; dec bh ; dec bl ; cmp bx, 0 ; per suggestion from @Chris Jester-Young use this: sub bx, 0101h je print_fizzbuzz test bl, bl je print_fizz test bh, bh je print_buzz print_other:

Naturally the various print_... routines would have to reset bh, bl or both as well as printing.

Improve formatting

Generally speaking, assembly language code is not indented in the way you have your code indented. It's much more linear, with the only indentation for assembly language statements or directives.

Consider better I/O

Your output routines are not shown, but it's likely that it would be more efficient to keep the numeric output in string form, incrementing each ASCII digit and emitting the string, rather than repeatedly converting from binary register contents to a string value.

\$\endgroup\$
4
  • 4
    \$\begingroup\$ Instead of dec bh; dec bl, you can just use sub bx, 101h. Then you can directly jz print_fizzbuzz without a cmp. The other comparisons can use test bl, bl and test bh, bh, to eliminate the literal 0s in the code (this makes the code smaller). \$\endgroup\$ Commented Jul 13, 2015 at 11:10
  • 3
    \$\begingroup\$ @ChrisJester-Young All excellent suggestions. Thanks! \$\endgroup\$ Commented Jul 13, 2015 at 11:53
  • \$\begingroup\$ Interesting hack to use partial registers for this, especially in a way that avoids reading a wider register after writing one of its halves (except when you reset only one of the counters). That would cause a partial-register stall on P6-family. (Why doesn't GCC use partial registers?). Another option is to unroll by 3 and only use one down-counter, like I played around with in this FizzBuzz answer where I stored bytes into a buffer for one print. \$\endgroup\$ Commented Dec 20, 2020 at 20:27
  • 1
    \$\begingroup\$ test reg,reg is preferred over cmp reg,0 - it's 1 byte shorter and sets FLAGS exactly equivalently in all cases (except for AF). Test whether a register is zero with CMP reg,0 vs OR reg,reg? \$\endgroup\$ Commented Dec 21, 2020 at 23:29
8
\$\begingroup\$

Use local labels

All of your labels are global labels.

Since all these labels are trying to complete the same task and they all work together, they should all be grouped under a single global variable, and have the rest of the labels be local.

For example, you would change this label:

fizzbuzz_check:

to:

.fizzbuzz_check:

Also, it's just better practice.

Different conditional jump

At the end of each check for either Fizz, Buzz, or FizzBuzz, you do something like this:

 je print_fizzbuzz ; if so, proceed with printing jmp fizz_check ; if not, try checking for fizz print_fizzbuzz:

This could be shortened to:

 jne main_loop print_fizzbuzz:

If the jne doesn't pass, execution will fall through to print_fizzbuzz

Versatility

Right now, your code only supports Fizz, Buzz, and Fizzbuzz.

But what if you wanted to change things up a bit? Say you wanted to say "Fizz" every fourth number?

To do this, you'd be adding quite a chunk of code.

Although, there is an easier way to do this; use strucs.

Say you created this struc:

struc message .say: resb 10 .num: resb 1 endstruc

You could then do something create a bunch of messages easily like this:

messages: db "FizzBuzz", 0, 0 db 15 db "Buzz",0,0,0,0,0,0 db 5 db "Fizz",0,0,0,0,0,0 db 3 db 0,0,0,0,0,0,0,0,0,0; so, when iterating, can know if the end has been reached db 0

(The extra 0's are for filling up the 10 bytes given for the name) (Note the order: you want greatest to least)

And, you can easily

Now, in your main code, you can easily iterate through messages and, if the counter is evenly divisible by the value in the num field, then you log the say field.

Now, the code could be written like this:

xor cx, cx main_loop: inc cx cmp cx, 100 jg .done call search jmp main_loop .done: .EXIT search: mov si, messages .next: mov ax, cx mov bh, [si + message.num]; divisor div bh cmp ah, 0; was evenly divisible je .print_message add si, message_size cmp byte [si], 0; the next item in `messages` is the terminator jne .next jmp .print_num .print_message: PutStr [si + message.say] nwln ret .print_num: PutInt cx nwln ret

Note: This was troublesome to test out without macros.s so if there are any issues, notify me

\$\endgroup\$
3
  • 2
    \$\begingroup\$ messages: \ .fizzbuzz: db "FizzBuzz",0 \ times 10 - ($ - .fizzbuzz) db 0 would be better for padding string fields to ten bytes long. \$\endgroup\$ Commented Dec 10, 2019 at 17:55
  • 1
    \$\begingroup\$ If you use .num == 0 as the terminator, you only have to load it once per iter (load the first iteration's divisor ahead of the first iteration, i.e. rotate and partially peel the loop to make that happen). Or with the current code, div byte [si + message.num] avoids wasting an instruction loading it into BH. Also, you can put .print_num before .print_message so the loop can fall into it. The main loop could also use a more efficient do{cx++; }while(cx<100); loop structure: Why are loops always compiled into "do...while" style (tail jump)?. \$\endgroup\$ Commented Dec 20, 2020 at 20:39
  • 1
    \$\begingroup\$ The code in search doesn't need to be a function, it could just be the loop body, even though it's large. That could be less readable, but if you want to maximize readability, use a compiler. Usually making code more general is the last thing you want to do when implementing in asm: take advantage of as many special cases as possible. Again, otherwise use a compiler so it can take advantage of fixed constants on the fly (like 2 and 4 being powers of 2, it wouldn't use div), unless the code truly needs to handle runtime variable arrays of divisors and messages. \$\endgroup\$ Commented Dec 20, 2020 at 20:43

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.