Getting performant VESA video output

[Ready4Dis]

Are you sure this has been optimised?

I gave him code to a much better optimized version that doesn't have an inner loop, which makes it much more optimized. Also, there is no need for ANY multiplication inside either loop, just simple addition.

[Brendan]

Hi,

Are you sure this has been optimised?

I gave him code to a much better optimized version that doesn't have an inner loop, which makes it much more optimized. Also, there is no need for ANY multiplication inside either loop, just simple addition.

Your version has an inner loop, it's just hidden inside "memcpy()".

The most recent disassembly looks like the original code (not your version), and does have both an inner and outer loop, but it's only the 32-bpp part of it (without the 24-bpp part), and without much optimisation done by the compiler; including not lifting things like the multiplication out of the inner loop, not optimising register usage effectively (and constantly doing local variable/stack accesses everywhere), etc.

There are some differences between this disassembly and the "known unoptimised" previous version - it's shorter, the "leave" was replaced with a (faster) "pop ebp; ret" pair; it's moving dwords (not bytes), etc.

Mostly, I'd describe it as "slightly optimised by compiler maybe" (it's almost like there's an "-O1.5" that's half way between no optimisation and some). If it wasn't for "leave" being replaced I would've assumed someone optimised the C++ source by hand and then compiled with optimisation disabled.

EDIT: I overlooked the fancy "NOP" instructions that were inserted too - they also indicate at least some optimisation was done. I tried cut & pasting the code and compiling it here (with GCC), except I converted it to C and filled in the missing structures/typedefs with crude hackjobs to make it compile. I tried "-Os", "-O2" and "-O3" and for all of these the compiler did lift the multiplication out of the inner loop and did "mostly reasonable" register allocation. Now I'm stumped - it was definitely compiled with optimisations, but trivial optimizations that GCC does do (and should have been done) just aren't happening.


Cheers,

Brendan

[Ready4Dis]

Your version has an inner loop, it's just hidden inside "memcpy()".

Semantics, memcpy isn't really a loop (as long as it maps to stos*, and doesn't do something silly like actually loop), like I said, semantics means you can either call it a loop or not . It's not a loop in the traditional sense, but you could consider rep stosb repeating a specific amount of times a loop. It doesn't loop like a cmp/jmp or the loop instruction does as it doesn't need to modify the PC (program counter).

Either way, you can easily remove the multiplication and such from the inner loop, see below:

For completeness, I separated it into two functions. Not tested, but should do the same same as yours.

Code: Select all

void vbe_video_output_t::blit24(g_rectangle invalid, g_rectangle sourceSize, g_color_argb* source)
{
	//Precalculate everything
	uint8_t* position = ((uint8_t*)video_mode_information.lfb) + (invalid.y * video_mode_information.bpsl) + (invalid.x * 3);
	uint8_t* p_source = (uint8_t*)source;

	uint32_t source_stride = (invalid.width - 1 - sourceSize.width) * 4;
	uint32_t position_stride = (invalid.y * video_mode_information.bpsl - (invalid.width - 1) * 3;

	for (int y = 0; y < invalid.height; y++)
	{
		//32-bit movs for all but the last pixel, since we don't want to overwrite any data by moving 32-bits into a 24-bit location
		for (int x = 0; x < invalid.width-1; x++)
		{
			*(uint32_t*)position = *(uint32_t*)p_source;
			position += 3;
			p_source += 4;
		}
		//Copy the last 24-bits (3 bytes), one 16-bit and one 8-bit mov
		*(uint16_t*)position = *(uint16_t*)p_source;
		position[2] = p_source[2];

		p_source += source_stride;
		position += video_mode_information.bpsl;
	}
}

void vbe_video_output_t::blit32(g_rectangle invalid, g_rectangle sourceSize, g_color_argb* source)
{
	//Precalculate everything
	uint8_t* position = ((uint8_t*)video_mode_information.lfb) + (invalid.y * video_mode_information.bpsl) + (invalid.x * 4);
	uint8_t* p_source = (uint8_t*)source;
	uint32_t source_stride = sourceSize.width * 4;   //Source is always 4-bits per pixel??
	uint32_t invalid_stride = invalid.width * 4;   //4 bits per pixel

												   //Now render each line, simple memcpy and two add's
	for (int y = 0; y < invalid.height; y++)
	{
		memcpy(position, p_source, invalid_stride);
		p_source += source_stride;
		position += video_mode_information.bpsl;
	}
}

I'm sure I could do better if I spent more than 5 minutes on it, but it should be pretty easy to beat the performance of the original without to much work. I only have MSVC on my work box right now, so no disassembly to look at.