It feels like the first comment I make for every assembler code review is the same: Add comments.
This may seem silly to you, after all, you know exactly what the code does. It's obvious, isn't it? But 6 weeks from now or 6 months from now, even 6 years from now, will it still be obvious to you what this code does? And that's just you, the person who wrote it. Reading someone else's assembler is always a challenge.
You are asking for a code review, and I would never sign off on code (assembler or otherwise) that didn't have decent comments describing what it does. If nothing else, performing that last pass thru the code describing what everything does sometimes turns up bugs or inefficiencies.
I realize this may just be a class project, or even just written for your own interest/education. But it's a habit you should acquire. Your future self (and the people you ask to read your code) will thank you.
So, let's start at the top with some file-scope comments. Perhaps something like this:
; qs.asm - Routine to perform a quicksort on a hard-coded
; list of numbers.
;
; Written for 16bit on i386.
;
; The list of numbers to sort is stored in the variable arr.
And looking at your first procedure, perhaps something like this:
; Swap - swap 2 variables in a list
;
; On input:
; sp + 4 - Pointer to first entry to swap
; sp + 2 - Pointer to second entry to swap
;
; On output:
; nothing
;
; Registers clobbered
; none
proc Swap
push bp ; Create stack frame
mov bp, sp
push di ; Preserve all used registers
push bx
push ax
push cx
xor ax, ax ; Zero register (not sure why you do this?)
xor cx, cx
mov bx, [bp+6] ; Get pointer to first entry
mov di, [bp+4] ; Get pointer to second entry
mov ah, [bx] ; Read first value
mov ch, [di] ; Read second value
mov [bx], ch ; Write second value to first location
mov [di], ah ; Write first value to second location
pop cx ; Restore used registers
pop ax
pop bx
pop di
pop bp ; Restore stack
ret
endp Swap
This is the same code as you posted above, just with comments and spacing. It makes the code drastically easier to understand and follow. And it's not like it makes the .exe file any bigger. It also highlights one of those inefficiencies I mentioned (why are you zeroing ax and cx?).
There are several other changes I would make to this routine.
- You do standard stack-frame code (
push bp mov bp, sp), but why? There is no benefit to doing this here, and it will make (what is likely to be) the busiest part of your code that much slower.
- Using
xchg, you can use 1 fewer register.
- Your routine uses pascal (or cdecl) calling convention (who
pops the registers that are passed to Swap?). There is nothing 'wrong' with calling functions this way, but there are alternatives. If you wanted to, you could follow the 'fastcall' convention, which passes parameters in registers instead of on the stack. Not having to read/write memory is typically faster, and requires fewer instructions (which is also faster). The fastcall rules also allow you to use ax without having to preserve it (since it's intended to be the return value). The downside is that if you call a routine that uses this convention, you need to make sure you don't mind having it stomp on the values of any of these registers.
I don't have a 16bit environment to test this, but given the comments, I expect you'll understand the intent, even if I muck up the syntax somewhere.
Implementing 1 & 2:
proc Swap
push ax ; Preserve all used registers
push bx
push cx
mov bx, [sp+4] ; Get pointer to first entry
mov cx, [sp+2] ; Get pointer to second entry
mov al, [bx] ; Read first value
xchg [cx], al ; xchg first value and second value
mov [bx], al ; Write second value back to first location
pop cx ; Restore used registers
pop bx
pop ax
ret
endp Swap
Note that xchg doesn't allow you to use 2 memory addresses, so you still need ax. But this still uses fewer registers, so needs fewer push/pops.
How about implementing #3? Instead of pushing the two pointers to be swapped onto the stack, what if we pass them in cx and dx? This is the normal 'fastcall' method on 32bit (IIRC, the standard for 16bit was a bit different). Likewise, ax is used to return the 'result.' In our case, there is no result, so we can use the register however we want.
Is changing the way functions are called kosher? Well... kinda. Assembler allows you to pass parameters however you want (as long as both the 'caller' and the 'callee' agree upon the rules), and the CPU certainly isn't going to care. On the other hand, if every routine you write uses a different method, it's going to make reading your code a nightmare (even with comments).
Generally what you want to do is pick 1 standard and use it for most or all of your code. Picking one of the industry standards makes sense. Obviously if your code needs to be callable from C, you must pick a standard that C supports. But since your entire program is asm, that doesn't apply here.
In this example, I'm using the Microsoft Fastcall standard for 32bit code. It's one I'm familiar with (although most of my recent code is 64bit). Obviously before you can use this, you'd need to change the code that calls Swap.
So, that gives us:
; Swap - swap 2 variables in a list
;
; On input:
; cx - Pointer to first entry to swap
; dx - Pointer to second entry to swap
;
; On output:
; nothing
;
; Registers clobbered
; ax
proc Swap
mov al, [cx] ; Read first value
xchg [dx], al ; xchg first value and second value
mov [cx], al ; Write second value back to first location
ret
endp Swap
You know what? The code is getting pretty small here. In fact, does it still make sense to have a routine for this? Or should you just put this code in Split? Putting this code 'inline' would make Split a bit bigger (since you call it 3 times), but avoiding the call/ret instructions can make things even faster. That's a judgement call.
I realize I've only covered the first routine, but this answer is already 5 pages long. That seems like enough to get you started.
Main takeaways should be:
- Comments.
- Either remove stack frames, or document why they are needed.
- Decide what calling convention to use. They each have benefits (preserve all registers, faster, callable from C, etc), so decide what is important to your program.
- Consider inlining smaller routines.
While Cody never responded to my request for clarification, he does tend to be right about these things.
Still, I'm not going to remove the xchg from this answer. Why? Because making this routine small makes you start thinking about inlining it. And once you think about inlining it, you look again at this code:
mov dh,[byte ptr bx]
cmp dh,[byte ptr di]
jae greater
push ax;offset to be swapped
push bx;offset to be swapped
call Swap
So before calling Swap, we not only had the pointers, we also had the values and knew they needed to be swapped. Instead of pushing the pointers, preserving registers, re-loading the pointers re-reading the values, and restoring the registers, we could simply have written the swapped values:
mov dh,[byte ptr bx] ; Read the first value
mov dl,[byte ptr di] ; Read the second value
cmp dh, dl
jae greater ; Are they correctly ordered?
mov [byte ptr bx], dl ; Nope. Swap them.
mov [byte ptr di], dh
So the question of how expensive xchg is drops out of the question. It was useful as a step along the way, even if ultimately it doesn't get used.
BTW, using a Swap routine in a higher-level language like C still makes sense. The C compiler can optimize this code and does the inlining for you. That way you get the benefit of clearer code (breaking out the Swap function makes the intent clear) while still getting the best performance.
Since assemblers aren't going "optimize" or "inline" for us, we as assembler programmers have to decide for ourselves where to make the trade-off between discrete functions and performance.
