[PATCH] D30677: [XRay][compiler-rt] Support TSC emulation even for x86_64

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: lib/xray/xray_inmemory_log.cc:141-150
+  __xray_InMemoryRawLog(FuncId, Type, [](uint8_t &CPU) XRAY_NEVER_INSTRUMENT {
+    timespec TS;
+    int result = clock_gettime(CLOCK_REALTIME, &TS);
+    if (result != 0) {
+      Report("clock_gettimg(2) return %d, errno=%d.", result, int(errno));
+      TS = {0, 0};
+    }
----------------
pelikan wrote:
> This lambda is already in xray_tsc.h as ALWAYS_INLINE.  I remember the discussion about how important it is to have it inlined, and here it's a copy+paste lambda with unclear inliner results.  Why can't we just use the readTSC function pointer here?
> 
> In that case we'll have to rename it to something like emulateTSC and move out of the #elif, as (in this case) readTSC is a real thing reading the real TSC.  And then even the other architectures' code would look clearer, because they *emulate* TSC, not *read* it.  Intel and Power would call "read" and others would call "emulate".
Considered it, and came to the conclusion that the alternative is a much more invasive change. I also don't want to preclude the implementation currently used in arm and mips from being able to use platform-specific features if/when we find out how to do them later (thereby imposing the burden on refactoring that to whoever is making that change later). The repetition here is fine and correct for x86_64 only. I'm attempting to localise the change here with a potentially more invasive refactoring to follow later.

The inlining question is easy to answer here -- `__xray::readTSC` is always inlined, so no function pointers (and indirect calls) were gotten in the making of this change. The template ensures that since all the definitions are inline and visible, there' no reason for the compiler to ignore those and not inline. It's the same rationale the standard library uses function templates for stuff that relies on inlining for performance purposes.


https://reviews.llvm.org/D30677