[PATCH] D30018: [XRay] Add __xray_customeevent(...) as a clang-supported builtin

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:2748
+    if (const auto *XRayAttr =
+            this->CurFuncDecl->getAttr<XRayInstrumentAttr>()) {
+      if (XRayAttr->neverXRayInstrument())
----------------
Don't need `this->`


================
Comment at: lib/CodeGen/CGBuiltin.cpp:2753-2754
+    Function *F = CGM.getIntrinsic(Intrinsic::xray_customevent);
+    // FIXME: assert that the types of the arguments match the intrinsic's
+    // specification.
+    SmallVector<Value *, 2> Args;
----------------
I don't think we need this FIXME. We don't generally assert things that are supposed to be guaranteed by semantic checking unless Sema proves to be unreliable.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:2755
+    // specification.
+    SmallVector<Value *, 2> Args;
+    auto FTy = F->getFunctionType();
----------------
We don't need a vector, CreateCall can take a std::initializer_list, so you can write `Builder.CreateCall(F, {Arg0Val, Arg1Val})`.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:2763
+      if (Arg0Ty->isArrayType())
+        Arg0Val = EmitArrayToPointerDecay(Arg0).getPointer();
+      else
----------------
OK, that is annoying, but it looks like it's what the other builtins do already. *shrug*


================
Comment at: lib/CodeGen/CGBuiltin.cpp:2773-2774
+    Args.push_back(Arg1);
+    Value *V = Builder.CreateCall(F, Args);
+    return RValue::get(V);
+  }
----------------
This is probably simpler as `return RValue::get(Builder.CreateCall(F, {Arg0Val, Arg1Val});`


================
Comment at: lib/Headers/xrayintrin.h:28
+extern "C" {
+inline void __xray_customevent(const char*, size_t) {
+  // Does nothing by design. The intent is the compiler's back-end will handle
----------------
I don't think you need this header. Also, the extern "C" would have to be guarded on __cplusplus.


https://reviews.llvm.org/D30018