preprocessor define to tell when rdtsc is available

RESOLVED FIXED

Status

--
enhancement
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 435308 [details] [diff] [review]
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?
Attachment #435308 - Flags: review?(nnethercote)
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.
(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.
(Assignee)

Comment 3

9 years ago
(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.
(Assignee)

Comment 4

9 years ago
Created attachment 435408 [details] [diff] [review]
adding AVMPLUS_HAS_RDTSC v2

The new version defines AVMPLUS_HAS_RDTSC explicitly for each supported architecture.
Attachment #435308 - Attachment is obsolete: true
Attachment #435408 - Flags: review?(nnethercote)
Attachment #435308 - Flags: review?(nnethercote)
Attachment #435408 - Flags: review?(nnethercote) → review+
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.
(Assignee)

Comment 6

9 years ago
(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 .
Whiteboard: checkin-needed

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/46b22e830aa9
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.