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

[Tim Shen via Phabricator via llvm-commits]

timshen added a comment.

Other than some nits and questions, looks good. :)



================
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]>;
----------------
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?


================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:885
 
+bool FastISel::selectXRayCustomEvent(const CallInst *I) {
+  const auto &Triple = TM.getTargetTriple();
----------------
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. :)


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1052
+  //   .p2align 1, ...
+  // .Lxray_event_sled_N:
+  //   jmp +N                    // jump across the call instruction
----------------
I believe that before at symbol, all caller-saved registers are actually saved?


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1075
+  // FIXME: Find another less hacky way do force the relative jump.
+  OutStreamer->EmitBytes("\xeb\x14");
+
----------------
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


https://reviews.llvm.org/D27503