Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

print.h: fix ODR #8 #1

Merged
merged 2 commits into from
Sep 1, 2024
Merged

print.h: fix ODR #8 #1

merged 2 commits into from
Sep 1, 2024

Conversation

GiorgosXou
Copy link

@GiorgosXou
Copy link
Author

GiorgosXou commented Aug 12, 2024

Now that I'm thinking about it, an array of pointers to functions, should be faster than switch-case.

@rilysh
Copy link

rilysh commented Sep 1, 2024

You're on a wrong repo, this is my fork for bb3f0cd

Now that I'm thinking about it, an array of pointers to functions, should be faster than switch-case.

It's considering a bad idea (for embedded systems). Because function pointers inside an array can't be inlined as regular functions, as unlike switch-case, where function gets inlined, because it's not creating an array of function pointers, and it doesn't have to keep track of the functions. This will create unnecessary multiple copies of functions where the function can be inlined, with little to no performance gain.

For example,
Using an array of function pointers

__attribute__((noinline))
static void call_array_ptr(int n)
{
	static void (*arr[6])() = {a, b, c, d, e, f};
	arr[n]();
}
call_array_ptr:
	movsxd	rcx, edi
	lea	rdx, [rip + call_array_ptr.arr]
	xor	eax, eax
	jmp	qword ptr [rdx + 8*rcx]

       # Multiple copies of a, with b, c, d labels.
a:
	mov	rax, qword ptr [rip + stdout@GOTPCREL]
	mov	rcx, qword ptr [rax]
	lea	rdi, [rip + .L.str]
	mov	esi, 2
	mov	edx, 1
	jmp	fwrite@PLT

Using a switch-statement

__attribute__((noinline))
static void switch_array_ptr(int n)
{
	switch (n) {
	case 1: a(); break;
	case 2: b(); break;
	case 3: c(); break;
	case 4: d(); break;
	case 5: e(); break;
	case 6: f(); break;
	default: break;
    }
}
switch_array_ptr:
	dec	edi
	cmp	edi, 5
	ja	.LBB1_1
        movsxd	rax, edi
	lea	rcx, [rip + .Lreltable.switch_array_ptr]
	movsxd	rdi, dword ptr [rcx + 4*rax]
	add	rdi, rcx
	mov	rax, qword ptr [rip + stdout@GOTPCREL]
	mov	rcx, qword ptr [rax]
	mov	esi, 2
	mov	edx, 1
	jmp	fwrite@PLT

       # No multiple copies, and every function called got inlined into a single copy.

At this point, you can notice that both are basically the similar.

@rilysh rilysh closed this Sep 1, 2024
@GiorgosXou
Copy link
Author

GiorgosXou commented Sep 1, 2024

You're on a wrong repo, this is my fork for bb3f0cd

@rilysh I don't think I'm at the wrong repo, I intentionally made a PR at your own fork because I agree with your position at c4e2f4e and exebook#9 (despite my suggestion of " ...an array of pointers to functions" [that was just a thought]). My PR consist of 2 completly different things from that suggestion\thought above: a commit about the ODR fix for exebook#8 and bool support

@rilysh
Copy link

rilysh commented Sep 1, 2024

I'm okay to merge this then, but I don't use this personally. I don't think I will add any features or do bug fixes in futures (as the author's repo is also inactive), but PRs are welcome.

Edit: Also for C++, it's probably better to use something else (e.g. fmtlib) than generic-print. As it doesn't have any type-safety to it's functions.

@rilysh rilysh reopened this Sep 1, 2024
@rilysh rilysh merged commit a6def90 into mystuffs:main Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants