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

[Martin Pelikán via Phabricator via llvm-commits]

pelikan added inline comments.


================
Comment at: include/xray/xray_log_interface.h:163
   void (*handle_arg0)(int32_t, XRayEntryType);
+  void (*handle_arg1)(int32_t, XRayEntryType, uint64_t);
 
----------------
dberris wrote:
> 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.
After our discussions about why we can't change this ABI (despite the fact LLVM itself is incredibly progressive in this regard), I've changed this to describe the proposed behaviour for anyone reading this code later.


================
Comment at: lib/xray/xray_fdr_logging.cc:196
 
+static void fdrReadTime(uint64_t &TSC, uint8_t &CPU) {
+  // Test once for required CPU features
----------------
dberris wrote:
> 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.
It was never there, fixed.  I don't see the point of "inline" since it's already static and because processFunctionHook is so huge, we obviously don't care about the function call overhead here.

Can we change this later, when that FIXME is being fixed properly?


================
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);
+}
----------------
dberris wrote:
> 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.
While I agree with you on the problems with the code structure, I tried making the change the least intrusive possible, and refactor later.  I'll take your comment as a permission to tear processFunctionHook apart.


https://reviews.llvm.org/D32844