58

First off, you have nice, clean, well-formatted, easy-to-read code. You have even included comments that explain the goal of each instruction. Too much of the time I review assembly-language code, these are the things that go wrong. You've gotten them all correct. Nice job! Now I don't have to pull my hair out trying to read and understand your code. ...


48

Try int x=n>255?255:n; ... x<0?0:x ... I'd expect this to produce mov eax,n cmp eax,255 cmovgt eax,255 ; conditional mov instruction test eax,eax cmovlt eax,0 If you are using MSVC SIX, you may not get the conditional move instruction. Try switching to a modern version of visual studio.


44

The comment ; *argv should be ; argv, since you are not yet dereferencing the pointer. After a cmp instruction, you should prefer je over jz, since it is nicer to the human reader. Oh, the old times, where you had to tell the assembler to jmp short because it couldn't figure it out on its own. :) In the run the BF program section, I would have changed esi ...


33

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. ...


28

Here's my attempt: unsigned char clamp(int n){ int a = 255; a -= n; a >>= 31; a |= n; n >>= 31; n = ~n; n &= a; return n; } It compiles to 7 instructions - which is the same as your current version. So it may or may not be faster. I haven't timed it though. But I think these are all single-cycle instructions. ...


27

Lots of review of the raw mechanics of your code. Not much review of the design. So here are just a few design tips. Use BPBs, because other people will. start: jmp main ; goto main There are two reasons that Volume Boot Record programs do this. The first and foremost is in order to skip over an embedded BIOS Parameter Block....


25

Style: Indent your operands to a consistent column, so mnemonics of different length don't make your code look so ragged. And use local .label labels inside functions. Comment code that depends on non-standard behaviour: stdout is only guaranteed to be line-buffered, and isn't automatically flushed when you read stdin in ISO C. Some systems (like Linux) ...


23

You are very inconsistent with how you clear a register: bgetkey: mov ax, 0 ; clear register a and main: mov ax, 0x0000 ; clear register a I recommend xoring the register with itself. Example: xor ax, ax As recommended by @icktoofay, in this section: bgetkey: mov ax, 0 ; clear ...


19

Unfortunately my hard drive containing my savegames crashed yesterday, so I cannot look up my solution, but I can say that the things you consider ugly are because of the limitations of that old computer system. I am abusing nodes as temporary storage all the time and my solution pretty much looked the same (if I remember correctly). As the histograms show ...


19

I see a number of things that may help you improve your code. Eliminate "magic numbers" This code has a number of "magic numbers," that is, unnamed constants such as 2, 0x0e, 0x10, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the ...


18

I echo most of SirPython’s recommendations (excepting possibly a detail about the recommended segment-setup code—I prefer setting up a flat memory space with all segment registers zero), but there’s a few other things I might change as well. I should note that a few of these may be more a matter of taste. Hexadecimal constants You are inconsistent with ...


18

There are numerous small things. You don't have to keep subtracting and adding 32 from rsp. Allocate the space once at the start of the function (main), reuse it for the duration, and add it back at the end (but see below). My personal preference would be to use mov ecx,offset question to make it clear that I want the address of the variable, and not the ...


17

You're not using cdq properly. cdq sign extends EAX into EDX usually in preparation for an idiv (signed division) instruction. However, you're using div (unsigned division), so instead of using cdq (which can potentially fill EDX with 0xFFFFFFFF), you should zero out that register instead. If you were doing an idiv, though, the cdq is in the wrong place ...


16

Here are some things that may help you improve your code. Omit C prolog and epilog if possible The part you have correctly labeled "C prologue" in your code is used by the compiler to be able to access local variables. However both inputs and outputs to this assembly routine are in registers, so no stack manipulations are needed. For that reason, we can ...


14

Conclusion 2011-12-05: I tried all of the suggestions again with VS 2010 Express. The generated code didn't change much, but the register assignments did which affected the overall results. A slight modification of the straightforward implementation suggested by Ira Baxter came up the winner. inline BYTE Clamp(int n) { n = n>255 ? 255 : n; ...


14

The comments in the clearmem procedure for the Linux block look a bit confusing. You could just have a summary of the procedure commented above, and have the individual comments for each line specify the meaning of the assembly instructions. Specifically, the lines that describe the C++ code don't quite reflect on the assembly code. clearmem itself doesn'...


14

First of all, yes, CortexM0 lacks any way to do 32x32=64 multiplication in hardware. CortexM3 and CortexM4 have the umull instruction, which lets you do 32x32=64 really easily. And yes, since you're writing in C, one possible implementation would be uint64_t mul32x32(uint32_t r0, uint32_t r1) { return r0*(uint64_t)r1; } but I assume you've already tried ...


13

I can't say I particularly like this code as it is right now. It seems to me that there are two reasonable approaches: if you think most of what you zero will be in main memory, then you probably just want the most compact code possible for the job. If you think it'll be used to zero data that might be in the cache a noticeable amount of the time, then you ...


13

Section sequence That's rather a matter of taste. Programmers used to 68k usually use the sequence .text, .data, .bss, programmers used to Java usually use the sequence .data, .bss, .text. In the assemblers known to me it doesn't make any difference, so it's rather a matter of taste or corporate policy. Actually, strictly speaking it's not the assembler ...


13

Wrong flag I believe you should be looking at the overflow flag instead of the carry flag, since all of your operands are signed values. On x86, the overflow flag is set if signed addition overflows. The carry flag is set if unsigned addition overflows. Not reliable As @Edward pointed out, it doesn't seem reliable to use this kind of function because ...


12

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 ...


12

The code is generally well written and easy to understand, but I have a few comments on it that could help improve it. Eliminate "magic numbers" In the StackCreate routine, the first instruction is mov rdi,24 but it's not clear what 24 signifies in this context. Either a comment or a named constant or both would help with that. Add more meaningful ...


12

One optimisation that would make your code run up to twice as fast, is to use the observation that a prime factor of n cannot be greater than n/2. So you can limit the number of times you iterate through your loop. Additionally, you know that once you have tested for divisibility by 2, then subsequent trial divisions can skip all even numbers. If you ...


12

Wrong increment First, there is a bug in your main loop. This line: add ebx, 1 should be: add ebx, 4 because you are operating on 4 floats at a time. Right now, you are doing array elements [0..3] following by [1..4]. On my computer the program crashed because it had a problem doing an unaligned load. Simplify divide by 4 This part ...


12

I have some ideas about how you might be able to improve your program. Avoid problems Rather than trying to deal with the problem for every instruction, one approach is avoiding it entirely. One way to do that is to simply append a number of bytes to the end of the vector. If the maximum bytes for an instruction is \$n\$, then append \$n-1\$ bytes to the ...


11

A few notes: Your included libraries and definitions at the beginning of your code is not very organized. #include <avr/io.h> //#undef __FLASH #ifndef __FLASH #include <avr/pgmspace.h> #define FLASH(x) const x PROGMEM #define FLASH_P(x) const x * const PROGMEM #define FLASH_PR(x, y) (x *)pgm_read_word(&(y)) #else #define FLASH(x) const ...


11

Everything from SyrPython and icktoofay, plus ... Four instructions: mov ah, 0x0e ; mov al, [si] ; cmp al, 0 ; check for null terminator jz print_end ; stop printing I would suggest: mov ax,0x0e00 ; clears al or al,[si] ; sets Z accordingly (if [si] is 0) jz ...


11

I see a number of things which could help you improve your code. Minimize register usage With assembly language programming, and in particular in embedded systems work, minimizing the use of resources is often vital. One of the most precious resources is the processor's registers. In this case there are only 32 of them, so minimizing their use is often ...


11

I see a number of things that may help you improve your code. Don't use const where it is forbidden Normally, I'd prefer to use const as you've done in this code. However, there are circumstances in C where it's not allowed and several of those instances appear in your code. The first is with the registers array in VM_CPU and the second is all of the ...


11

I see some things that may help you improve your code. Don't leak memory The code for InitializeVM allocate memory for the vm->memory member. Every allocation should have a corresponding free, so I would suggest adding this function to the interface and then calling it at the end of main: void DestroyVM(TOYVM* vm) { free(vm->memory); } Check ...


Only top voted, non community-wiki answers of a minimum length are eligible