555338 - preprocessor define to tell when rdtsc is available

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, enhancement)

Description

[Igor Bukanov]

Attachment: adding AVMPLUS_HAS_RDTSC

js/src/nanojit/avmplus.h defines the rdtsc function for number of platforms. But it does not provide a convenient macro to tell if the function is available. That makes it hard to use the function in the profiling code. 

To fix that the attached patch adds AVMPLUS_HAS_RDTSC and sets it 0 or 1 depending on rdtsc availability.

To Nicholas:are you the right person to review this?

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 435308 [details] [diff] [review]
adding AVMPLUS_HAS_RDTSC

>+#else
>+
>+# define AVMPLUS_HAS_RDTSC 0
>+
> #endif /* architecture */
> 
>+#ifndef AVMPLUS_HAS_RDTSC
>+# define AVMPLUS_HAS_RDTSC 1
>+#endif

Ugh, I don't like this.  I'd much prefer just putting
  
  #define AVMPLUS_HAS_RDTSC 1

in the appropriate #if branches.  It's more verbose but simpler.

Also, you haven't indicated where you would use this #define.  Would it be simpler to just define rdtsc() on platforms that don't support it and make it return 0?  I see that we also have _rdtsc() in vprof/vprof.cpp and that's how it works.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> 
> Also, you haven't indicated where you would use this #define.

Ah, I see it's bug 553682.

> Would it be simpler to just define rdtsc() on platforms that don't support
> it and make it return 0?

I'd still like an answer on that.

Comment 3

[Igor Bukanov]

(In reply to comment #1)
> Would it be simpler to just define rdtsc() on platforms that don't support
> it and make it return 0?

In the bug 553682 I want to report a compile-time error when a user tries to enable a rdtsc-based GC profiling on a platform where rdtsc is not available. Reporting 0 during such profiling would just waste developers time.

Comment 4

[Igor Bukanov]

Attachment: adding AVMPLUS_HAS_RDTSC v2

The new version defines AVMPLUS_HAS_RDTSC explicitly for each supported architecture.

Comment 5

[Nicholas Nethercote [inactive]]

You know you have to land this on nanojit-central first, right?  (See https://developer.mozilla.org/en/NanojitMerge.)  I'll be doing an NJ-to-TM merge tomorrow.

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> You know you have to land this on nanojit-central first, right?  (See
> https://developer.mozilla.org/en/NanojitMerge.)  

Could you land the patch then? I do not have permission to commit to hg.mozilla.org/projects/nanojit-central .

Comment 7

[Nicholas Nethercote [inactive]]

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/673444e26f48

TM: http://hg.mozilla.org/tracemonkey/rev/46b22e830aa9

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/46b22e830aa9