[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
This revision was automatically updated to reflect the committed changes.
Closed by commit rC329985: [XRay][clang] Add flag to choose instrumentation
bundles (authored by dberris, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D44970?vs=142315&id=142317#toc
Repository:
rC Clang
https://reviews.llvm.org/D44970
Files:
include/clang/Basic/XRayInstr.h
include/clang/Driver/Options.td
include/clang/Driver/XRayArgs.h
include/clang/Frontend/CodeGenOptions.h
lib/Basic/CMakeLists.txt
lib/Basic/XRayInstr.cpp
lib/CodeGen/CGBuiltin.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Driver/XRayArgs.cpp
lib/Frontend/CompilerInvocation.cpp
test/CodeGen/xray-instrumentation-bundles.cpp
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -1846,9 +1846,10 @@
StringRef Category) const {
if (!LangOpts.XRayInstrument)
return false;
+
const auto &XRayFilter = getContext().getXRayFilter();
using ImbueAttr = XRayFunctionFilter::ImbueAttribute;
- auto Attr = XRayFunctionFilter::ImbueAttribute::NONE;
+ auto Attr = ImbueAttr::NONE;
if (Loc.isValid())
Attr = XRayFilter.shouldImbueLocation(Loc, Category);
if (Attr == ImbueAttr::NONE)
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -3341,6 +3341,11 @@
case Builtin::BI__xray_customevent: {
if (!ShouldXRayInstrumentFunction())
return RValue::getIgnored();
+
+if (!CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
+XRayInstrKind::Custom))
+ return RValue::getIgnored();
+
if (const auto *XRayAttr = CurFuncDecl->getAttr())
if (XRayAttr->neverXRayInstrument() && !AlwaysEmitXRayCustomEvents())
return RValue::getIgnored();
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -468,7 +468,10 @@
/// AlwaysEmitXRayCustomEvents - Return true if we should emit IR for calls to
/// the __xray_customevent(...) builin calls, when doing XRay instrumentation.
bool CodeGenFunction::AlwaysEmitXRayCustomEvents() const {
- return CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents;
+ return CGM.getCodeGenOpts().XRayInstrumentFunctions &&
+ (CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents ||
+ CGM.getCodeGenOpts().XRayInstrumentationBundle.Mask ==
+ XRayInstrKind::Custom);
}
llvm::Constant *
@@ -900,7 +903,9 @@
}
// Apply xray attributes to the function (as a string, for now)
- bool InstrumentXray = ShouldXRayInstrumentFunction();
+ bool InstrumentXray = ShouldXRayInstrumentFunction() &&
+CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
+XRayInstrKind::Function);
if (D && InstrumentXray) {
if (const auto *XRayAttr = D->getAttr()) {
if (XRayAttr->alwaysXRayInstrument())
Index: lib/Driver/XRayArgs.cpp
===
--- lib/Driver/XRayArgs.cpp
+++ lib/Driver/XRayArgs.cpp
@@ -58,8 +58,7 @@
}
} else {
D.Diag(diag::err_drv_clang_unsupported)
- << (std::string(XRayInstrumentOption) +
- " on non-supported target OS");
+ << (std::string(XRayInstrumentOption) + " on " + Triple.str());
}
XRayInstrument = true;
if (const Arg *A =
@@ -82,6 +81,36 @@
options::OPT_fnoxray_link_deps, true))
XRayRT = false;
+auto Bundles =
+Args.getAllArgValues(options::OPT_fxray_instrumentation_bundle);
+if (Bundles.empty())
+ InstrumentationBundle.Mask = XRayInstrKind::All;
+else
+ for (const auto &B : Bundles) {
+llvm::SmallVector BundleParts;
+llvm::SplitString(B, BundleParts, ",");
+for (const auto &P : BundleParts) {
+ // TODO: Automate the generation of the string case table.
+ auto Valid = llvm::StringSwitch(P)
+ .Cases("none", "all", "function", "custom", true)
+ .Default(false);
+
+ if (!Valid) {
+D.Diag(clang::diag::err_drv_invalid_value)
+<< "-fxray-instrumentation-bundle=" << P;
+continue;
+ }
+
+ auto Mask = parseXRayInstrValue(P);
+ if (Mask == XRayInstrKind::None) {
+InstrumentationBundle.clear();
+break;
+ }
+
+ InstrumentationBundle.Mask |= Mask;
+}
+ }
+
// Validate the always/never attribute files. We also make sure that they
// are treated as actual dependencies.
for (const auto &Filename :
@@ -165,7 +194,7 @@
CmdArgs.push_back(Args.MakeArgSt
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
dberris updated this revision to Diff 142315.
dberris marked 5 inline comments as done.
dberris added a comment.
- fixup: Address comments
https://reviews.llvm.org/D44970
Files:
clang/include/clang/Basic/XRayInstr.h
clang/include/clang/Driver/Options.td
clang/include/clang/Driver/XRayArgs.h
clang/include/clang/Frontend/CodeGenOptions.h
clang/lib/Basic/CMakeLists.txt
clang/lib/Basic/XRayInstr.cpp
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/Driver/XRayArgs.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/CodeGen/xray-instrumentation-bundles.cpp
Index: clang/test/CodeGen/xray-instrumentation-bundles.cpp
===
--- /dev/null
+++ clang/test/CodeGen/xray-instrumentation-bundles.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=none -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=custom -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function,custom -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function \
+// RUN: -fxray-instrumentation-bundle=custom -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,CUSTOM %s
+
+// CHECK: define void @_Z16alwaysInstrumentv() #[[ALWAYSATTR:[0-9]+]] {
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ static constexpr char kPhase[] = "always";
+ __xray_customevent(kPhase, 6);
+ // CUSTOM: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+ // NOCUSTOM-NOT: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+}
+
+// FUNCTION: attributes #[[ALWAYSATTR]] = {{.*}} "function-instrument"="xray-always" {{.*}}
+// NOFUNCTION-NOT: attributes #[[ALWAYSATTR]] = {{.*}} "function-instrument"="xray-always" {{.*}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -26,6 +26,7 @@
#include "clang/Basic/VersionTuple.h"
#include "clang/Basic/VirtualFileSystem.h"
#include "clang/Basic/Visibility.h"
+#include "clang/Basic/XRayInstr.h"
#include "clang/Config/config.h"
#include "clang/Driver/DriverDiagnostic.h"
#include "clang/Driver/Options.h"
@@ -75,9 +76,9 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/Process.h"
#include "llvm/Support/Regex.h"
+#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/Target/TargetOptions.h"
-#include "llvm/Support/ScopedPrinter.h"
#include
#include
#include
@@ -446,6 +447,25 @@
}
}
+static void parseXRayInstrumentationBundle(StringRef FlagName, StringRef Bundle,
+ ArgList &Args, DiagnosticsEngine &D,
+ XRayInstrSet &S) {
+ llvm::SmallVector BundleParts;
+ llvm::SplitString(Bundle, BundleParts, ",");
+ for (const auto B : BundleParts) {
+auto Mask = parseXRayInstrValue(B);
+if (Mask == XRayInstrKind::None)
+ if (B != "none")
+D.Report(diag::err_drv_invalid_value) << FlagName << Bundle;
+ else
+S.Mask = Mask;
+else if (Mask == XRayInstrKind::All)
+ S.Mask = Mask;
+else
+ S.set(Mask, true);
+ }
+}
+
// Set the profile kind for fprofile-instrument.
static void setPGOInstrumentor(CodeGenOptions &Opts, ArgList &Args,
DiagnosticsEngine &Diags) {
@@ -820,11 +840,23 @@
Args.hasArg(OPT_finstrument_functions_after_inlining);
Opts.InstrumentFunctionEntryBare =
Args.hasArg(OPT_finstrument_function_entry_bare);
- Opts.XRayInstrumentFunctions = Args.hasArg(OPT_fxray_instrument);
+
+ Opts.XRayInstrumentFunctions =
+ Args.hasArg(OPT_fxray_instrument);
Opts.XRayAlwaysEmitCustomEvents =
Args.hasArg(OPT_fxray_always_emit_customevents);
Opts.XRayInstructionThreshold =
getLastArgIntValue(Args, OPT_fxray_instruction_threshold_EQ, 200, Diags);
+
+ auto XRayInstrBundles =
+ Args.getAllArgValues(OPT_fxray_instrumentation_bundle);
+
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
dberris added a comment.
Thanks, Martin! Landing now, after suggested changes.
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:471
bool CodeGenFunction::AlwaysEmitXRayCustomEvents() const {
- return CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents;
+ return CGM.getCodeGenOpts().XRayInstrumentFunctions &&
+ (CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents ||
pelikan wrote:
> I kind of don't like how the "-fxray-instrument" variable is called
> "XRayInstrumentFunctions" because that's not what it means any more. I think
> in a later diff, we should clean this up. Or maybe even clean up some of the
> old flags whose functionality has been superseded by this. But the logic
> here is fine.
>
> Same with the misleading "ShouldXRayInstrumentFunction()" which controls
> custom events too, and not just functions.
Good point. Yes, we could make this cleaner.
https://reviews.llvm.org/D44970
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
pelikan accepted this revision.
pelikan added a comment.
This revision is now accepted and ready to land.
Most of my comments are minor enough I'd be OK if this went in. But please
consider them before committing.
Comment at: clang/include/clang/Driver/XRayArgs.h:29
std::vector Modes;
+ XRayInstrSet XRayInstrumentationBundle;
bool XRayInstrument = false;
Since the class already has "XRay" in its name, I would rename the member to
just "InstrumentationBundle", just as most of the other members are.
Comment at: clang/include/clang/Frontend/CodeGenOptions.h:111
+ enum XRayInstrumentationTypes {
+XRay_Function,
Now I fail to spot where is this enum used. IIUC this should work even when
it's not here, as the code uses the things in namespace XRayInstrKind.
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:469
/// AlwaysEmitXRayCustomEvents - Return true if we should emit IR for calls to
/// the __xray_customevent(...) builin calls, when doing XRay instrumentation.
bool CodeGenFunction::AlwaysEmitXRayCustomEvents() const {
typo: builin -> builtin
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:471
bool CodeGenFunction::AlwaysEmitXRayCustomEvents() const {
- return CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents;
+ return CGM.getCodeGenOpts().XRayInstrumentFunctions &&
+ (CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents ||
I kind of don't like how the "-fxray-instrument" variable is called
"XRayInstrumentFunctions" because that's not what it means any more. I think
in a later diff, we should clean this up. Or maybe even clean up some of the
old flags whose functionality has been superseded by this. But the logic here
is fine.
Same with the misleading "ShouldXRayInstrumentFunction()" which controls custom
events too, and not just functions.
Comment at: clang/lib/Driver/XRayArgs.cpp:107-109
+ } else
+D.Diag(clang::diag::err_drv_invalid_value)
+<< "-fxray-instrumentation-bundle=" << P;
nitpick: I'd rewrite it to
if (!Valid) {
D.Diag
continue; // or break or return
}
but the current code logic is fine.
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:457
+auto Mask = parseXRayInstrValue(B);
+if (Mask == 0)
+ if (B != "none")
did you mean: "(Mask == None)"?
https://reviews.llvm.org/D44970
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
dberris updated this revision to Diff 141958.
dberris marked an inline comment as done.
dberris added a comment.
Herald added a subscriber: mgorny.
- Rebase
- Address comments to use a bitmask/bitset for instrumentation bundle
https://reviews.llvm.org/D44970
Files:
clang/include/clang/Basic/XRayInstr.h
clang/include/clang/Driver/Options.td
clang/include/clang/Driver/XRayArgs.h
clang/include/clang/Frontend/CodeGenOptions.h
clang/lib/Basic/CMakeLists.txt
clang/lib/Basic/XRayInstr.cpp
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/Driver/XRayArgs.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/CodeGen/xray-instrumentation-bundles.cpp
Index: clang/test/CodeGen/xray-instrumentation-bundles.cpp
===
--- /dev/null
+++ clang/test/CodeGen/xray-instrumentation-bundles.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=none -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=custom -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM %s
+
+// CHECK: define void @_Z16alwaysInstrumentv() #[[ALWAYSATTR:[0-9]+]] {
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ static constexpr char kPhase[] = "always";
+ __xray_customevent(kPhase, 6);
+ // CUSTOM: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+ // NOCUSTOM-NOT: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+}
+
+// FUNCTION: attributes #[[ALWAYSATTR]] = {{.*}} "function-instrument"="xray-always" {{.*}}
+// NOFUNCTION-NOT: attributes #[[ALWAYSATTR]] = {{.*}} "function-instrument"="xray-always" {{.*}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -26,6 +26,7 @@
#include "clang/Basic/VersionTuple.h"
#include "clang/Basic/VirtualFileSystem.h"
#include "clang/Basic/Visibility.h"
+#include "clang/Basic/XRayInstr.h"
#include "clang/Config/config.h"
#include "clang/Driver/DriverDiagnostic.h"
#include "clang/Driver/Options.h"
@@ -75,9 +76,9 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/Process.h"
#include "llvm/Support/Regex.h"
+#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/Target/TargetOptions.h"
-#include "llvm/Support/ScopedPrinter.h"
#include
#include
#include
@@ -446,6 +447,25 @@
}
}
+static void parseXRayInstrumentationBundle(StringRef FlagName, StringRef Bundle,
+ ArgList &Args, DiagnosticsEngine &D,
+ XRayInstrSet &S) {
+ llvm::SmallVector BundleParts;
+ llvm::SplitString(Bundle, BundleParts, ",");
+ for (const auto B : BundleParts) {
+auto Mask = parseXRayInstrValue(B);
+if (Mask == 0)
+ if (B != "none")
+D.Report(diag::err_drv_invalid_value) << FlagName << Bundle;
+ else
+S.Mask = Mask;
+else if (Mask == XRayInstrKind::All)
+ S.Mask = Mask;
+else
+ S.set(Mask, true);
+ }
+}
+
// Set the profile kind for fprofile-instrument.
static void setPGOInstrumentor(CodeGenOptions &Opts, ArgList &Args,
DiagnosticsEngine &Diags) {
@@ -821,11 +841,23 @@
Args.hasArg(OPT_finstrument_functions_after_inlining);
Opts.InstrumentFunctionEntryBare =
Args.hasArg(OPT_finstrument_function_entry_bare);
- Opts.XRayInstrumentFunctions = Args.hasArg(OPT_fxray_instrument);
+
+ Opts.XRayInstrumentFunctions =
+ Args.hasArg(OPT_fxray_instrument);
Opts.XRayAlwaysEmitCustomEvents =
Args.hasArg(OPT_fxray_always_emit_customevents);
Opts.XRayInstructionThreshold =
getLastArgIntValue(Args, OPT_fxray_instruction_threshold_EQ, 200, Diags);
+
+ auto XRayInstrBundles =
+ Args.getAllArgValues(OPT_fxray_instrumentation_bundle);
+ if (XRayInstrBundles.empty())
+Opts.XRayInstrumentationBundle.Mask = XRayInstrKind::All;
+ else
+for (const auto &A : XRayInstrBundles)
+ parseXRayInstrumentationBundle("-fxray-instrumentation-bundle=", A, Args,
+ Diags, Opts.XRayInstrumentationBundle);
+
Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
Opts.CallFEntry = Args.hasArg(OPT_mfentry);
Opts.EmitOpenCLArgMetadata = Args.hasArg(OPT_cl_kernel_arg_info);
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
echristo added inline comments.
Comment at: clang/include/clang/Frontend/CodeGenOptions.h:110
+ enum XRayInstrumentationPointBundle {
+XRay_All, // Always emit all the instrumentation points.
dberris wrote:
> pelikan wrote:
> > To me, this feels like a bitfield would be a better match.
> > All = Function | Custom
> > None = 0
> Thought about that, but the ergonomics from the user-side isn't as good.
> Having to know about which kinds of sleds specifically to enable seems much
> harder to explain. Using bundles that we control from the beginning keeps
> this much simpler.
I dunno, I think I agree with the other commenter. This feels awkward.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3245
+
+using XRayBundles = CodeGenOptions::XRayInstrumentationPointBundle;
+auto Bundle = CGM.getCodeGenOpts().getXRayInstrumentationBundle();
Break this out into some sort of function to determine which one we want? We do
this a couple times.
https://reviews.llvm.org/D44970
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
dberris updated this revision to Diff 140735.
dberris added a comment.
- fixup: Fix tests for better coverage of settings
https://reviews.llvm.org/D44970
Files:
clang/include/clang/Driver/Options.td
clang/include/clang/Driver/XRayArgs.h
clang/include/clang/Frontend/CodeGenOptions.def
clang/include/clang/Frontend/CodeGenOptions.h
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/Driver/XRayArgs.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/CodeGen/xray-instrumentation-bundles.cpp
Index: clang/test/CodeGen/xray-instrumentation-bundles.cpp
===
--- /dev/null
+++ clang/test/CodeGen/xray-instrumentation-bundles.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=none -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,NOCUSTOM %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function-extents -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=custom-only -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,NOFUNCTION,CUSTOM %s
+
+// CHECK: define void @_Z16alwaysInstrumentv() #[[ALWAYSATTR:[0-9]+]] {
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ static constexpr char kPhase[] = "always";
+ __xray_customevent(kPhase, 6);
+ // CUSTOM: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+ // NOCUSTOM-NOT: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+}
+
+// FUNCTION: attributes #[[ALWAYSATTR]] = {{.*}} "function-instrument"="xray-always" {{.*}}
+// NOFUNCTION-NOT: attributes #[[ALWAYSATTR]] = {{.*}} "function-instrument"="xray-always" {{.*}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -446,6 +446,23 @@
}
}
+static CodeGenOptions::XRayInstrumentationPointBundle
+parseXRayInstrumentationBundle(Arg *A, ArgList &Args, DiagnosticsEngine &D) {
+ StringRef V = A->getValue();
+ auto Bundle = llvm::StringSwitch(V)
+ .Case("all", CodeGenOptions::XRayInstrumentationPointBundle::XRay_All)
+ .Case("none", CodeGenOptions::XRayInstrumentationPointBundle::XRay_None)
+ .Case("function-extents",
+CodeGenOptions::XRayInstrumentationPointBundle::XRay_Function)
+ .Case("custom-only",
+CodeGenOptions::XRayInstrumentationPointBundle::XRay_CustomOnly)
+ .Default(CodeGenOptions::XRayInstrumentationPointBundle::XRay_All);
+ if (Bundle == CodeGenOptions::XRayInstrumentationPointBundle::XRay_All &&
+ !V.empty() && V != "all")
+D.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << V;
+ return Bundle;
+}
+
// Set the profile kind for fprofile-instrument.
static void setPGOInstrumentor(CodeGenOptions &Opts, ArgList &Args,
DiagnosticsEngine &Diags) {
@@ -821,11 +838,18 @@
Args.hasArg(OPT_finstrument_functions_after_inlining);
Opts.InstrumentFunctionEntryBare =
Args.hasArg(OPT_finstrument_function_entry_bare);
- Opts.XRayInstrumentFunctions = Args.hasArg(OPT_fxray_instrument);
+
+ Opts.XRayInstrumentFunctions =
+ Args.hasArg(OPT_fxray_instrument);
Opts.XRayAlwaysEmitCustomEvents =
Args.hasArg(OPT_fxray_always_emit_customevents);
Opts.XRayInstructionThreshold =
getLastArgIntValue(Args, OPT_fxray_instruction_threshold_EQ, 200, Diags);
+
+ if (auto A = Args.getLastArg(OPT_fxray_instrumentation_bundle))
+Opts.setXRayInstrumentationBundle(
+parseXRayInstrumentationBundle(A, Args, Diags));
+
Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
Opts.CallFEntry = Args.hasArg(OPT_mfentry);
Opts.EmitOpenCLArgMetadata = Args.hasArg(OPT_cl_kernel_arg_info);
Index: clang/lib/Driver/XRayArgs.cpp
===
--- clang/lib/Driver/XRayArgs.cpp
+++ clang/lib/Driver/XRayArgs.cpp
@@ -27,6 +27,8 @@
constexpr char XRayInstrumentOption[] = "-fxray-instrument";
constexpr char XRayInstructionThresholdOption[] =
"-fxray-instruction-threshold=";
+constexpr char XRayInstrumentationBundleOption[] =
+"-fxray-instrumentation-bundle=";
} // namespace
XRayArgs::XRayArgs(const ToolChain &TC, const ArgList &Args) {
@@ -50,10 +52,10 @@
<< (std::string(XRayInstrumentOption) + " on " + Triple.str());
}
} else if (Triple.getOS() == llvm::Triple::FreeBSD) {
-if (Triple.getArch() != llvm::Triple::x86_64) {
- D.Diag(diag::err_
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
dberris added inline comments.
Comment at: clang/include/clang/Frontend/CodeGenOptions.h:110
+ enum XRayInstrumentationPointBundle {
+XRay_All, // Always emit all the instrumentation points.
pelikan wrote:
> To me, this feels like a bitfield would be a better match.
> All = Function | Custom
> None = 0
Thought about that, but the ergonomics from the user-side isn't as good. Having
to know about which kinds of sleds specifically to enable seems much harder to
explain. Using bundles that we control from the beginning keeps this much
simpler.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3248
+// custom events.
+{
+ auto XRayBundle = CGM.getCodeGenOpts().getXRayInstrumentationBundle();
echristo wrote:
> I'd probably spell this code like the block above it rather than this.
The codegen options don't work like pointers, and we can't use C++17 if
initialisers yet. Would have loved this to be:
```
if (auto XRayBundle = ...; XRayBundle == ... || XRayBundle == ...)
return RValue::getIgnored();
```
Removed the scoping instead.
Comment at: clang/lib/Driver/XRayArgs.cpp:62
+ << (std::string(XRayInstrumentOption) +
+ " on non-supported target OS");
}
pelikan wrote:
> I would also print the triple. Something like:
>
> "-fomit-bugs is not supported on target win64-ppc-none"
>
> will be much more informative, especially when you collect logs from build
> machines on lots of architectures (like Linux distro/BSD package builders do).
Probably not today. Good idea though, could be a different patch.
https://reviews.llvm.org/D44970
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
dberris updated this revision to Diff 140731.
dberris marked 8 inline comments as done.
dberris added a comment.
- fixup: address comments
https://reviews.llvm.org/D44970
Files:
clang/include/clang/Driver/Options.td
clang/include/clang/Driver/XRayArgs.h
clang/include/clang/Frontend/CodeGenOptions.def
clang/include/clang/Frontend/CodeGenOptions.h
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/Driver/XRayArgs.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/CodeGen/xray-instrumentation-bundles.cpp
Index: clang/test/CodeGen/xray-instrumentation-bundles.cpp
===
--- /dev/null
+++ clang/test/CodeGen/xray-instrumentation-bundles.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=none -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function-extents -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=none -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes CHECK,DISABLE %s
+
+// CHECK-LABEL: alwaysInstrument
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ static constexpr char kPhase[] = "always";
+ __xray_customevent(kPhase, 6);
+ // CHECK-NOT: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+ // DISABLE-NOT: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+}
+
+// DISABLE-NOT: function-instrument=always
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -446,6 +446,23 @@
}
}
+static CodeGenOptions::XRayInstrumentationPointBundle
+parseXRayInstrumentationBundle(Arg *A, ArgList &Args, DiagnosticsEngine &D) {
+ StringRef V = A->getValue();
+ auto Bundle = llvm::StringSwitch(V)
+ .Case("all", CodeGenOptions::XRayInstrumentationPointBundle::XRay_All)
+ .Case("none", CodeGenOptions::XRayInstrumentationPointBundle::XRay_None)
+ .Case("function-extents",
+CodeGenOptions::XRayInstrumentationPointBundle::XRay_Function)
+ .Case("custom-only",
+CodeGenOptions::XRayInstrumentationPointBundle::XRay_CustomOnly)
+ .Default(CodeGenOptions::XRayInstrumentationPointBundle::XRay_All);
+ if (Bundle == CodeGenOptions::XRayInstrumentationPointBundle::XRay_All &&
+ !V.empty() && V != "all")
+D.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << V;
+ return Bundle;
+}
+
// Set the profile kind for fprofile-instrument.
static void setPGOInstrumentor(CodeGenOptions &Opts, ArgList &Args,
DiagnosticsEngine &Diags) {
@@ -821,11 +838,20 @@
Args.hasArg(OPT_finstrument_functions_after_inlining);
Opts.InstrumentFunctionEntryBare =
Args.hasArg(OPT_finstrument_function_entry_bare);
- Opts.XRayInstrumentFunctions = Args.hasArg(OPT_fxray_instrument);
Opts.XRayAlwaysEmitCustomEvents =
Args.hasArg(OPT_fxray_always_emit_customevents);
Opts.XRayInstructionThreshold =
getLastArgIntValue(Args, OPT_fxray_instruction_threshold_EQ, 200, Diags);
+
+ if (auto A = Args.getLastArg(OPT_fxray_instrumentation_bundle))
+Opts.setXRayInstrumentationBundle(
+parseXRayInstrumentationBundle(A, Args, Diags));
+
+ Opts.XRayInstrumentFunctions =
+ Args.hasArg(OPT_fxray_instrument) &&
+ Opts.getXRayInstrumentationBundle() !=
+ CodeGenOptions::XRayInstrumentationPointBundle::XRay_None;
+
Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
Opts.CallFEntry = Args.hasArg(OPT_mfentry);
Opts.EmitOpenCLArgMetadata = Args.hasArg(OPT_cl_kernel_arg_info);
Index: clang/lib/Driver/XRayArgs.cpp
===
--- clang/lib/Driver/XRayArgs.cpp
+++ clang/lib/Driver/XRayArgs.cpp
@@ -27,6 +27,8 @@
constexpr char XRayInstrumentOption[] = "-fxray-instrument";
constexpr char XRayInstructionThresholdOption[] =
"-fxray-instruction-threshold=";
+constexpr char XRayInstrumentationBundleOption[] =
+"-fxray-instrumentation-bundle=";
} // namespace
XRayArgs::XRayArgs(const ToolChain &TC, const ArgList &Args) {
@@ -50,10 +52,10 @@
<< (std::string(XRayInstrumentOption) + " on " + Triple.str());
}
} else if (Triple.getOS() == llvm::Triple::FreeBSD) {
-if (Triple.getArch() != llvm::Triple::x86_64) {
- D.Diag(diag::err_drv_clang_unsupported)
- << (std::string(XRayInstrumentOption) + " on " + Triple.str());
-}
+ if (Triple.getArch() != llvm::Triple::x86_64) {
+D.Diag(diag:
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
pelikan added a comment.
I would probably add more tests for the different configurations, but I suspect
more diffs are coming after this.
Comment at: clang/include/clang/Driver/Options.td:1112
+ Group, Flags<[CC1Option]>,
+ HelpText<"Select which bundle of XRay instrumentation points to emit.
Options: all, none, function-extents, custom-only.">;
+
I'd suggest making them "fn-only" and "custom-only", or just plain "function"
and "custom".
Comment at: clang/include/clang/Driver/Options.td:1112
+ Group, Flags<[CC1Option]>,
+ HelpText<"Select which bundle of XRay instrumentation points to emit.
Options: all, none, function-extents, custom-only.">;
+
pelikan wrote:
> I'd suggest making them "fn-only" and "custom-only", or just plain "function"
> and "custom".
I also don't get why "none" is an option here, if it's equivalent to
-fnoxray-instrument. Why duplicate the functionality and add more points the
users will have to debug?
Comment at: clang/include/clang/Frontend/CodeGenOptions.h:110
+ enum XRayInstrumentationPointBundle {
+XRay_All, // Always emit all the instrumentation points.
To me, this feels like a bitfield would be a better match.
All = Function | Custom
None = 0
Comment at: clang/lib/Driver/XRayArgs.cpp:62
+ << (std::string(XRayInstrumentOption) +
+ " on non-supported target OS");
}
I would also print the triple. Something like:
"-fomit-bugs is not supported on target win64-ppc-none"
will be much more informative, especially when you collect logs from build
machines on lots of architectures (like Linux distro/BSD package builders do).
Comment at: clang/lib/Driver/XRayArgs.cpp:86
+Args.getLastArg(options::OPT_fxray_instrumentation_bundle)) {
+ StringRef B = A->getValue();
+ if (B != "all" && B != "none" && B != "function-extents" &&
echristo wrote:
> How about a more descriptive name here and a string switch below?
+1
https://reviews.llvm.org/D44970
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
echristo added inline comments.
Comment at: clang/include/clang/Frontend/CodeGenOptions.h:113
+XRay_None,// Emit none of the instrumentation points.
+XRay_FunctionExtents, // Only emit function entry/exit instrumentation
+ // points.
"function" might spell easier? :)
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3248
+// custom events.
+{
+ auto XRayBundle = CGM.getCodeGenOpts().getXRayInstrumentationBundle();
I'd probably spell this code like the block above it rather than this.
Comment at: clang/lib/Driver/XRayArgs.cpp:61
D.Diag(diag::err_drv_clang_unsupported)
- << (std::string(XRayInstrumentOption) + " on non-supported target
OS");
+ << (std::string(XRayInstrumentOption) +
+ " on non-supported target OS");
Extraneous reformat.
Comment at: clang/lib/Driver/XRayArgs.cpp:86
+Args.getLastArg(options::OPT_fxray_instrumentation_bundle)) {
+ StringRef B = A->getValue();
+ if (B != "all" && B != "none" && B != "function-extents" &&
How about a more descriptive name here and a string switch below?
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:452
+ StringRef V = A->getValue();
+ if (V == "all")
+return CodeGenOptions::XRayInstrumentationPointBundle::XRay_All;
StringSwitch maybe?
https://reviews.llvm.org/D44970
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles
dberris created this revision.
dberris added reviewers: echristo, kpw, eizan, pelikan.
This change addresses http://llvm.org/PR36926 by allowing users to pick
which instrumentation bundles to use, when instrumenting with XRay. In
particular, the flag `-fxray-instrumentation-bundle=` has four valid
values:
- `all`: the default, which will emit all kinds of instrumentation
points.
- `none`: equivalent to -fnoxray-instrument
- `function-extents`: only emits the entry/exit sleds
- `custom-only`: only emits the custom event sleds
https://reviews.llvm.org/D44970
Files:
clang/include/clang/Driver/Options.td
clang/include/clang/Driver/XRayArgs.h
clang/include/clang/Frontend/CodeGenOptions.def
clang/include/clang/Frontend/CodeGenOptions.h
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/Driver/XRayArgs.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/CodeGen/xray-instrumentation-bundles.cpp
Index: clang/test/CodeGen/xray-instrumentation-bundles.cpp
===
--- /dev/null
+++ clang/test/CodeGen/xray-instrumentation-bundles.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fxray-instrument -fxray-instrumentation-bundle=none -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN: -fxray-instrumentation-bundle=function-extents -x c++ \
+// RUN: -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN: | FileCheck %s
+
+// CHECK-LABEL: alwaysInstrument
+[[clang::xray_always_instrument]] void alwaysInstrument() {
+ static constexpr char kPhase[] = "always";
+ __xray_customevent(kPhase, 6);
+ // CHECK-NOT: call void @llvm.xray.customevent(i8*{{.*}}, i32 6)
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -446,6 +446,21 @@
}
}
+static CodeGenOptions::XRayInstrumentationPointBundle
+parseXRayInstrumentationBundle(Arg *A, ArgList &Args, DiagnosticsEngine &D) {
+ StringRef V = A->getValue();
+ if (V == "all")
+return CodeGenOptions::XRayInstrumentationPointBundle::XRay_All;
+ if (V == "none")
+return CodeGenOptions::XRayInstrumentationPointBundle::XRay_None;
+ if (V == "function-extents")
+return CodeGenOptions::XRayInstrumentationPointBundle::XRay_FunctionExtents;
+ if (V == "custom-only")
+return CodeGenOptions::XRayInstrumentationPointBundle::XRay_CustomOnly;
+ D.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << V;
+ return CodeGenOptions::XRayInstrumentationPointBundle::XRay_All;
+}
+
// Set the profile kind for fprofile-instrument.
static void setPGOInstrumentor(CodeGenOptions &Opts, ArgList &Args,
DiagnosticsEngine &Diags) {
@@ -853,6 +868,11 @@
Args.hasArg(OPT_fxray_always_emit_customevents);
Opts.XRayInstructionThreshold =
getLastArgIntValue(Args, OPT_fxray_instruction_threshold_EQ, 200, Diags);
+
+ if (auto A = Args.getLastArg(OPT_fxray_instrumentation_bundle))
+Opts.setXRayInstrumentationBundle(
+parseXRayInstrumentationBundle(A, Args, Diags));
+
Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
Opts.CallFEntry = Args.hasArg(OPT_mfentry);
Opts.EmitOpenCLArgMetadata = Args.hasArg(OPT_cl_kernel_arg_info);
Index: clang/lib/Driver/XRayArgs.cpp
===
--- clang/lib/Driver/XRayArgs.cpp
+++ clang/lib/Driver/XRayArgs.cpp
@@ -27,6 +27,8 @@
constexpr char XRayInstrumentOption[] = "-fxray-instrument";
constexpr char XRayInstructionThresholdOption[] =
"-fxray-instruction-threshold=";
+constexpr char XRayInstrumentationBundleOption[] =
+"-fxray-instrumentation-bundle=";
} // namespace
XRayArgs::XRayArgs(const ToolChain &TC, const ArgList &Args) {
@@ -50,13 +52,14 @@
<< (std::string(XRayInstrumentOption) + " on " + Triple.str());
}
} else if (Triple.getOS() == llvm::Triple::FreeBSD) {
-if (Triple.getArch() != llvm::Triple::x86_64) {
- D.Diag(diag::err_drv_clang_unsupported)
- << (std::string(XRayInstrumentOption) + " on " + Triple.str());
-}
+ if (Triple.getArch() != llvm::Triple::x86_64) {
+D.Diag(diag::err_drv_clang_unsupported)
+<< (std::string(XRayInstrumentOption) + " on " + Triple.str());
+ }
} else {
D.Diag(diag::err_drv_clang_unsupported)
- << (std::string(XRayInstrumentOption) + " on non-supported target OS");
+ << (std::string(XRayInstrumentOption) +
+ " on non-supported target OS");
}
XRayInstrument = true;
if (const Arg *A =
@@ -75,6 +78,19 @@
options::OPT_fnoxray_always_emit_customevents, false))
XRayAlwaysEmitCustomEvents = true;
+// We check the bundle of instrumentation tha
