This is an archive of the discontinued LLVM Phabricator instance.

[XRay] Add __xray_customeevent(...) as a clang-supported builtin
ClosedPublic

Authored by dberris on Feb 15 2017, 6:30 PM.

Details

Summary

We define the __xray_customeevent builtin that gets translated to
IR calls to the correct intrinsic. The default implementation of this is
a no-op function. The codegen side of this follows the following logic:

  • When -fxray-instrument is not provided in the driver, we elide all calls to __xray_customevent.
  • When -fxray-instrument is enabled and a function is marked as "never instrumented", we elide all calls to __xray_customevent in that function; if either marked as "always instrumented" or subject to threshold-based instrumentation, we emit a call to the llvm.xray.customevent intrinsic from LLVM for each __xray_customevent occurrence in the function.

This change depends on D27503 (to land in LLVM first).

Diff Detail

Repository
rL LLVM

Event Timeline

dberris created this revision.Feb 15 2017, 6:30 PM
dberris edited the summary of this revision. (Show Details)Feb 15 2017, 6:31 PM
dberris added a reviewer: rnk.Mar 21 2017, 7:26 PM
dberris added a subscriber: rnk.

@rnk -- if you have time, a review for this would be appreciated too.

rnk added inline comments.Mar 22 2017, 11:07 AM
lib/CodeGen/CGBuiltin.cpp
2748 ↗(On Diff #88641)

Don't need this->

2753–2754 ↗(On Diff #88641)

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.

2755 ↗(On Diff #88641)

We don't need a vector, CreateCall can take a std::initializer_list, so you can write Builder.CreateCall(F, {Arg0Val, Arg1Val}).

2763 ↗(On Diff #88641)

OK, that is annoying, but it looks like it's what the other builtins do already. *shrug*

2773–2774 ↗(On Diff #88641)

This is probably simpler as return RValue::get(Builder.CreateCall(F, {Arg0Val, Arg1Val});

lib/Headers/xrayintrin.h
28 ↗(On Diff #88641)

I don't think you need this header. Also, the extern "C" would have to be guarded on __cplusplus.

dberris updated this revision to Diff 98129.May 8 2017, 12:17 AM
dberris marked 6 inline comments as done.

Address comments.

lib/Headers/xrayintrin.h
28 ↗(On Diff #88641)

Reverted the file.

rnk accepted this revision.May 8 2017, 9:00 AM

lgtm

lib/CodeGen/CGBuiltin.cpp
2748 ↗(On Diff #88641)

Do you think this-> is necessary?

This revision is now accepted and ready to land.May 8 2017, 9:00 AM
dberris marked an inline comment as done.May 8 2017, 5:58 PM

Thanks @rnk!

lib/CodeGen/CGBuiltin.cpp
2748 ↗(On Diff #88641)

Nope, just force of habit (and an aid to autocomplete) :D

This revision was automatically updated to reflect the committed changes.
dberris marked an inline comment as done.