[PATCH] D27503: [XRay] Custom event logging intrinsic

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: include/llvm/IR/Intrinsics.td:779
+// Takes a pointer to a string and the length of the string.
+def int_xray_customevent : Intrinsic<[], [llvm_ptr_ty, llvm_i8_ty],
+                                     [NoCapture<0>, ReadOnly<0>, IntrWriteMem]>;
----------------
timshen wrote:
> Is the length going to be i8 forever? What if the strings are getting longer? Would it be better to use a word-size length variable?
good point -- i32 is definitely better for future proofing.


================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:885
 
+bool FastISel::selectXRayCustomEvent(const CallInst *I) {
+  const auto &Triple = TM.getTargetTriple();
----------------
timshen wrote:
> I thought It's not necessary to have a FastISel implementation for this, because if FastISel doesn't work, the execution falls back to SelectionDAG.
> 
> Now that you already have this, I'm not object to keeping it though. :)
Ah, cool. That's good to know. :)


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1052
+  //   .p2align 1, ...
+  // .Lxray_event_sled_N:
+  //   jmp +N                    // jump across the call instruction
----------------
timshen wrote:
> I believe that before at symbol, all caller-saved registers are actually saved?
I thought that too, except the calling convention for the intrinsic seems to be the C calling convention (default) as opposed to the SysV64 calling convention that I'd like it to be. This is where the question of whether it's possible (or whether it makes sense) for us to actually provide the calling convention for the intrinsic (or whether the front-end has to explicitly state this).

Any suggestions on how to force the calling convention here?


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1075
+  // FIXME: Find another less hacky way do force the relative jump.
+  OutStreamer->EmitBytes("\xeb\x14");
+
----------------
timshen wrote:
> Maybe jump to an "exit" symbol, and emit that symbol at the end of the sled?
> 
> See: https://github.com/llvm-mirror/llvm/blob/288180ac40ab050711298f42340233bd47916634/lib/Target/PowerPC/PPCAsmPrinter.cpp#L1076
That doesn't quite work for x86 because the jump instruction emitted can vary (i.e. it's not always 2 bytes). The runtime requires us to be able to write two bytes atomically to enable/disable the jump. Since there's no way yet for us to force a specific jump instruction that will be preserved when the relaxation pass runs, for now we need to hard-code this static relative jump ourselves.


https://reviews.llvm.org/D27503