push EAX;
Why are you pushing EAX here? There is no need to do that. EAX is a caller-save register, which means that the callee (i.e., your function) is free to clobber it. You aren't required to preserve its value.
(EAX, EDX, and ECX are the caller-save registers in the Win32 ABI; the others are callee-save.)
The only other reason to push a register would be to align the stack, but that is not necessary here, either. The stack will already be aligned correctly when control passes to your function.
xor EAX, EAX;
I presume you know that this is a commonly-used trick for clearing a register (XORing it with itself). However, you do not need to pre-clear a register before you MOVe a value into it.
mov EAX,color;
This line is wrong; that is what the assembler error is telling you. color is passed to this function as a reference-to-DWORD, but under the hood, references are implemented as pointers, so it is actually passed as a pointer-to-DWORD. Which means that you cannot access the value of color directly—you must use pointer indirection (or "indirect addressing" in x86 parlance). Since you are using inline assembly, you can let the compiler do the stack bookkeeping for you and just refer to the memory location by the name of the formal parameter:
mov EAX, DWORD PTR [color] ; get the address of color
mov EAX, DWORD PTR [EAX] ; dereference it, storing the result in EAX
Of course, since you are not actually modifying color inside of this function, there is no reason to pass it as a reference parameter. In general, scalar values (e.g., integers) should always be passed by value, rather than by reference, unless you actually need the reference. This is both more efficient and more readable—an optimizing compiler will pass the value in a register, making this pointer indirection and its attendant cost completely unnecessary.
mov R, AL;
Here, the assembler is giving you an "operand size conflict" error. Because R is actually a reference, implemented as a pointer, it is 32-bits. It is a 32-bit pointer that points to an 8-bit location in memory. So you are trying to move an 8-bit value (AL) into a 32-bit location (the pointer). The operands are different sizes. So once again, you have to use indirect addressing. It looks just like the code above, except that now R is byte-sized and you need to use a different register as a scratch register in order to avoid clobbering the value in EAX we worked so hard to get there:
mov EDX, DWORD PTR [R] ; get the address of R
mov BYTE PTR [EDX], AL ; dereference it so we can store AL in there
This moves the low byte of EAX (which we can refer to as AL) into the byte-sized memory location designated by R.
Same thing for the next line, except that now you're moving the high byte of EAX (referred to as AH). We can reuse EDX now here, because we don't need its old value again:
mov EDX, DWORD PTR [G] ; get the address of G
mov BYTE PTR [EDX], AH ; dereference it so we can store AH in there
shr EAX, 16;
This is correct.
mov B, AL;
Third verse, same as the first. As you now know, this should be:
mov EDX, DWORD PTR [B] ; get the address of B
mov BYTE PTR [EDX], AL ; dereference it so we can store AL in there
pop EAX;
Popping EAX is now unnecessary, since we didn't push EAX at the beginning.
Putting it all together, then, you get the following sequence of instructions:
void Get_RGB_color(const DWORD &color, uint8_t &R, uint8_t & G, uint8_t & B)
{
__asm
{
mov EAX, DWORD PTR [color]
mov EAX, DWORD PTR [EAX]
mov EDX, DWORD PTR [R]
mov BYTE PTR [EDX], AL
mov EDX, DWORD PTR [G]
mov BYTE PTR [EDX], AH
shr EAX, 16
mov EDX, DWORD PTR [B]
mov BYTE PTR [EDX], AL
}
}
However, this is not the most optimal way to write the code. Accessing the low and high 8 bits of a 32-bit register, while permissible, is slow. An optimizing compiler would avoid this, and in the process, avoid the need for a shift instruction:
void Get_RGB_color(const DWORD &color, uint8_t &R, uint8_t & G, uint8_t & B)
{
__asm
{
mov EAX, DWORD PTR [color] ; get the address of color
mov EAX, DWORD PTR [EAX] ; get the value in EAX
mov EDX, DWORD PTR [R] ; get the address of R
mov CL, BYTE PTR [EAX] ; get the value of the lowest byte (8 bits) of color
mov BYTE PTR [EDX], CL ; dereference R and store that byte in it
mov EDX, DWORD PTR [G] ; get the address of G
mov CL, BYTE PTR [EAX + 1] ; get the value of the second-to-lowest byte in color
mov BYTE PTR [EDX], CL ; dereference G and store that byte in it
mov EDX, DWORD PTR [B] ; get the address of B
mov CL, BYTE PTR [EAX + 2] ; get the value of the third-to-lowest byte in color
mov BYTE PTR [EDX], CL ; dereference B and store that byte in it
}
}
But there are still partial register stalls lurking in there to slow things down. So a really smart compiler would eliminate those by either pre-zeroing the registers or using movzx:
void Get_RGB_color(const DWORD &color, uint8_t &R, uint8_t & G, uint8_t & B)
{
__asm
{
mov EAX, DWORD PTR [color]
mov EAX, DWORD PTR [EAX]
mov EDX, DWORD PTR [R]
movzx ECX, BYTE PTR [EAX]
mov BYTE PTR [EDX], CL
mov EDX, DWORD PTR [G]
movzx ECX, BYTE PTR [EAX + 1]
mov BYTE PTR [EDX], CL
mov EDX, DWORD PTR [B]
movzx ECX, BYTE PTR [EAX + 2]
mov BYTE PTR [EDX], CL
}
}
It might also reorder the instructions and cleverly allocate registers to parallelize the three operations as much as possible. There are doubtless even more efficient ways of doing this. Unless you're trying to learn assembly language programming (in which case, using the inline assembler doesn't make much sense), strongly prefer to write the code like this:
void Get_RGB_color(const DWORD &color, uint8_t &R, uint8_t & G, uint8_t & B)
{
R = (color & 0xFF);
G = ((color >> 8) & 0xFF);
B = ((color >> 16) & 0xFF);
}
One final note: you do not need to end each assembly language instruction with a semicolon—even when using the inline assembly syntax. That's only necessary in C and C++. The reason why it doesn't matter, though, because a semicolon is actually a comment delimiter in assembler, so it would be like writing the following in C:
int foo;/**/
colorby value, rather than by reference. That way, the value can simply be passed in a register. – Cody Gray