[PATCH] D32844: [XRay] [compiler-rt] FDR logging arg1 handler

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a reviewer: kpw.
dberris added a subscriber: kpw.
dberris added a comment.

Adding in @kpw who also has more context on this implementation.



================
Comment at: include/xray/xray_log_interface.h:163
   void (*handle_arg0)(int32_t, XRayEntryType);
+  void (*handle_arg1)(int32_t, XRayEntryType, uint64_t);
 
----------------
Because of ABI reasons, we really shouldn't change this struct. The comments say that if implementations need to add their arg1 handler, they should do it in the init implementation.


================
Comment at: lib/xray/xray_fdr_logging.cc:196
 
+static void fdrReadTime(uint64_t &TSC, uint8_t &CPU) {
+  // Test once for required CPU features
----------------
You dropped the `XRAY_NEVER_INSTRUMENT`?

Also now you probably want to mark this always inline.

It would also make it more efficient to just return a 16-byte struct, so the inliner doesn't have to guess whether TSC and/or CPU alias.


================
Comment at: lib/xray/xray_fdr_logging.cc:224-225
+  fdrReadTime(TSC, CPU);
+  __xray_fdr_internal::processFunctionHook(FuncId, Entry, 0, TSC, CPU,
+                                           clock_gettime, LoggingStatus, BQ);
+}
----------------
I'm ambivalent about this API change. Let me try to explain why.

First, while the compiler might tail-call into the specified function, it was already using 7 arguments. In SysV64, that means the first 6 arguments that fit in registers will be in the registers already. Adding an argument spills more stuff onto the stack. That's one.

Second, we're paying this cost in the path where we know we don't have an argument. That seems like a waste to me.

Maybe it's better to refactor this differently, so we have the innards of `processFunctionHook` extracted appropriately so we can handle the "we are logging arguments" case and "we are not logging arguments" better?

Or at least be able to compose either of these functions in an easier manner so while there will be a little redundancy, it's going to be easier to read and reason about. Packing on more arguments to the already overloaded `processFunctionHook` function seems like a bad way to go especially if we intend to support multiple argument logging in the future.


https://reviews.llvm.org/D32844