phire / doom3.gpl

Doom 3 GPL source release
GNU General Public License v3.0
15 stars 4 forks source link

Sys_GetClockTicks() defective on x86_64 #11

Open DanielGibson opened 12 years ago

DanielGibson commented 12 years ago

https://github.com/phire/doom3.gpl/blob/master/neo/sys/linux/main.cpp#L259 This code doesn't work. (Probably because unsigned long is 64bits long on Linux/amd64, but rdtsc uses uint32_t values). Use the following (from Kai Blaschke in the iodoom3 mailing list) instead:

uint32_t lo, hi;

__asm__ __volatile__ (      // serialize
"xorl %%eax,%%eax \n        cpuid"
::: "%rax", "%rbx", "%rcx", "%rdx");
/* We cannot use "=A", since this would use %rax on x86_64 and return only the lower 32bits of the TSC */
__asm__ __volatile__ ("rdtsc" : "=a" (lo), "=d" (hi));
return (double) lo + (double) 0xFFFFFFFF * hi; 

Cheers,

phire commented 12 years ago

This issue raises a number of questions.

Ok, first I'm dubious about a solution that that involves embedded asm. Shouldn't there be a library call that does about the same thing.

Second, A quick look over the rest of the code raises questions about this code's ablity to handle modern CPUs with dynamic frequency scaling. Once again, Library call....

Third, Why am I not subscribed to the mailing list? I'm sure I signed up after it was first setup, before there were any posts. Anyway I should be signed up now.

DanielGibson commented 12 years ago

I don't know of any library, but maybe there is a solution. It should be able to handle dynamic frequency scaling at least on some CPUs that support it, but it may not handle multiple cores well..

This code seems to be used mostly in profiling (through idTimer) and even then it's almost always used to calculate a time (by dividing through the frequency) I think. It would probably be better if idTimer used Sys_GetMilliseconds() or something like that.

Anyway: the code I posted is less buggy then the one currently used and behaves like the original code for x86.