[cfe-commits] [PATCH]: GPROF implementation
Daniel Dunbar
daniel at zuster.org
Sat Feb 5 09:26:19 PST 2011
Hi Roman!
On Mon, Jan 24, 2011 at 9:10 AM, Roman Divacky <rdivacky at freebsd.org> wrote:
> Hi,
>
> The attached patch implemented GPROF (ie. -pg) support in clang.
Awesome!
Some comments:
--
> diff --git a/include/clang/Driver/CC1Options.td b/include/clang/Driver/CC1Options.td
> index 813ee5b..1c751b7 100644
> --- a/include/clang/Driver/CC1Options.td
> +++ b/include/clang/Driver/CC1Options.td
> @@ -187,6 +187,7 @@ def mms_bitfields : Flag<"-mms-bitfields">,
> HelpText<"Set the default structure layout to be compatible with the Microsoft compiler standard.">;
> def O : Joined<"-O">, HelpText<"Optimization level">;
> def Os : Flag<"-Os">, HelpText<"Optimize for size">;
> +def pg : Flag<"-pg">, HelpText<"GPROF">;
This help text should be something like "enable mcount instrumentation
(for gprof)"
> //===----------------------------------------------------------------------===//
> // Dependency Output Options
> diff --git a/include/clang/Frontend/CodeGenOptions.h b/include/clang/Frontend/CodeGenOptions.h
> index 216743d..4438874 100644
> --- a/include/clang/Frontend/CodeGenOptions.h
> +++ b/include/clang/Frontend/CodeGenOptions.h
> @@ -58,6 +58,7 @@ public:
> /// hidden visibility.
> unsigned InstrumentFunctions : 1; /// Set when -finstrument-functions is
> /// enabled.
> + unsigned GPROFInstrument : 1; /// Set when -pg is enabled
> unsigned LessPreciseFPMAD : 1; /// Enable less precise MAD instructions to be
> /// generated.
> unsigned MergeAllConstants : 1; /// Merge identical constants.
I think this should be called InstrumentForProfiling.
> @@ -127,6 +128,7 @@ public:
> HiddenWeakTemplateVTables = 0;
> HiddenWeakVTables = 0;
> InstrumentFunctions = 0;
> + GPROFInstrument = 0;
> LessPreciseFPMAD = 0;
> MergeAllConstants = 1;
> NoCommon = 0;
> diff --git a/lib/CodeGen/CodeGenFunction.cpp b/lib/CodeGen/CodeGenFunction.cpp
> index d887943..9e11348 100644
> --- a/lib/CodeGen/CodeGenFunction.cpp
> +++ b/lib/CodeGen/CodeGenFunction.cpp
> @@ -210,6 +210,45 @@ void CodeGenFunction::EmitFunctionInstrumentation(const char *Fn) {
> CallSite);
> }
>
> +void CodeGenFunction::EmitGPROFInstrumentation() {
This function should be named something more accurate, like
EmitMCountInstrumentation().
> + if (!CGM.getCodeGenOpts().GPROFInstrument)
> + return;
I prefer that tests like this be at the call site, not at the caller. As
written, this function should be called MaybeEmitMCountInstrumentation(), if
that makes sense?
> +
> + const char *mcountName = NULL;
> +
> + // TODO: fill in more stuff
> + switch (Target.getTriple().getOS()) {
> + default:
> + mcountName = ".mcount";
> + break;
> + case llvm::Triple::FreeBSD:
> + switch (Target.getTriple().getArch()) {
> + default:
> + case llvm::Triple::x86:
> + case llvm::Triple::x86_64:
> + mcountName = ".mcount";
> + break;
> + case llvm::Triple::mips:
> + case llvm::Triple::mipsel:
> + case llvm::Triple::ppc:
> + case llvm::Triple::ppc64:
> + mcountName = "_mcount";
> + break;
> + case llvm::Triple::arm:
> + mcountName = "__mcount";
> + break;
> +
> + }
> + break;
> + }
Please make this a target hook, instead of having a local switch on
target stuff here.
Targets should just set the McountFunctionName in the target info constructor,
if it differs from the default.
I believe the appropriate default is mcount, FWIW, not ".mcount".
> +
> + llvm::FunctionType *FTy =
> + llvm::FunctionType::get(llvm::Type::getVoidTy(VMContext), false);
> +
> + llvm::Constant *MCountFn = CGM.CreateRuntimeFunction(FTy, mcountName);
> + Builder.CreateCall(MCountFn);
> +}
> +
> void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
> llvm::Function *Fn,
> const FunctionArgList &Args,
> @@ -260,6 +299,8 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
>
> EmitFunctionInstrumentation("__cyg_profile_func_enter");
>
> + EmitGPROFInstrumentation();
> +
> // FIXME: Leaked.
> // CC info is ignored, hopefully?
> CurFnInfo = &CGM.getTypes().getFunctionInfo(FnRetTy, Args,
> diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h
> index d1794d0..a3dc8ad 100644
> --- a/lib/CodeGen/CodeGenFunction.h
> +++ b/lib/CodeGen/CodeGenFunction.h
> @@ -1066,6 +1066,9 @@ public:
> /// function instrumentation is enabled.
> void EmitFunctionInstrumentation(const char *Fn);
>
> + /// EmitGPROFInstrumentation - Emit all to .mcount.
> + void EmitGPROFInstrumentation();
> +
> /// EmitFunctionProlog - Emit the target specific LLVM code to load the
> /// arguments for the given function. This is also responsible for naming the
> /// LLVM function arguments.
> diff --git a/lib/Driver/Tools.cpp b/lib/Driver/Tools.cpp
> index f0a142f..68a3447 100644
> --- a/lib/Driver/Tools.cpp
> +++ b/lib/Driver/Tools.cpp
> @@ -1308,6 +1308,7 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
> Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
> Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
> Args.AddLastArg(CmdArgs, options::OPT_flimit_debug_info);
> + Args.AddLastArg(CmdArgs, options::OPT_pg);
>
> // -flax-vector-conversions is default.
> if (!Args.hasFlag(options::OPT_flax_vector_conversions,
> @@ -1738,13 +1739,10 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
>
> C.addCommand(new Command(JA, *this, Exec, CmdArgs));
>
> - // Explicitly warn that these options are unsupported, even though
> - // we are allowing compilation to continue.
> - for (arg_iterator it = Args.filtered_begin(options::OPT_pg),
> - ie = Args.filtered_end(); it != ie; ++it) {
> - (*it)->claim();
> - D.Diag(clang::diag::warn_drv_clang_unsupported) << (*it)->getAsString(Args);
> - }
> + if (Arg *A = Args.getLastArg(options::OPT_pg))
> + if (Args.hasArg(options::OPT_fomit_frame_pointer))
> + D.Diag(clang::diag::err_drv_argument_not_allowed_with)
> + << A->getAsString(Args) << "-fomit-frame-pointer";
I think this error makes more sense if the order is reverse, i.e.,
clang: error: invalid argument '-fomit-frame-pointer' not allowed with '-pg'
>
> // Claim some arguments which clang supports automatically.
>
> @@ -3176,8 +3174,12 @@ void freebsd::Link::ConstructJob(Compilation &C, const JobAction &JA,
> if (!Args.hasArg(options::OPT_nostdlib) &&
> !Args.hasArg(options::OPT_nostartfiles)) {
> if (!Args.hasArg(options::OPT_shared)) {
> - CmdArgs.push_back(Args.MakeArgString(
> - getToolChain().GetFilePath("crt1.o")));
> + if (Args.hasArg(options::OPT_pg))
> + CmdArgs.push_back(Args.MakeArgString(
> + getToolChain().GetFilePath("gcrt1.o")));
> + else
> + CmdArgs.push_back(Args.MakeArgString(
> + getToolChain().GetFilePath("crt1.o")));
> CmdArgs.push_back(Args.MakeArgString(
> getToolChain().GetFilePath("crti.o")));
> CmdArgs.push_back(Args.MakeArgString(
> @@ -3205,13 +3207,21 @@ void freebsd::Link::ConstructJob(Compilation &C, const JobAction &JA,
> !Args.hasArg(options::OPT_nodefaultlibs)) {
> if (D.CCCIsCXX) {
> getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs);
> - CmdArgs.push_back("-lm");
> + if (Args.hasArg(options::OPT_pg))
> + CmdArgs.push_back("-lm_p");
> + else
> + CmdArgs.push_back("-lm");
> }
> // FIXME: For some reason GCC passes -lgcc and -lgcc_s before adding
> // the default system libraries. Just mimic this for now.
> - CmdArgs.push_back("-lgcc");
> + if (Args.hasArg(options::OPT_pg))
> + CmdArgs.push_back("-lgcc_p");
> + else
> + CmdArgs.push_back("-lgcc");
> if (Args.hasArg(options::OPT_static)) {
> CmdArgs.push_back("-lgcc_eh");
> + } else if (Args.hasArg(options::OPT_pg)) {
> + CmdArgs.push_back("-lgcc_eh_p");
> } else {
> CmdArgs.push_back("--as-needed");
> CmdArgs.push_back("-lgcc_s");
> @@ -3219,12 +3229,26 @@ void freebsd::Link::ConstructJob(Compilation &C, const JobAction &JA,
> }
>
> if (Args.hasArg(options::OPT_pthread))
> - CmdArgs.push_back("-lpthread");
> - CmdArgs.push_back("-lc");
> + if (Args.hasArg(options::OPT_pg))
> + CmdArgs.push_back("-lpthread_p");
> + else
> + CmdArgs.push_back("-lpthread");
> +
> + if (Args.hasArg(options::OPT_pg)) {
> + if (Args.hasArg(options::OPT_shared))
> + CmdArgs.push_back("-lc");
> + else
> + CmdArgs.push_back("-lc_p");
> + CmdArgs.push_back("-lgcc_p");
> + } else {
> + CmdArgs.push_back("-lc");
> + CmdArgs.push_back("-lgcc");
> + }
>
> - CmdArgs.push_back("-lgcc");
> if (Args.hasArg(options::OPT_static)) {
> CmdArgs.push_back("-lgcc_eh");
> + } else if (Args.hasArg(options::OPT_pg)) {
> + CmdArgs.push_back("-lgcc_eh_p");
> } else {
> CmdArgs.push_back("--as-needed");
> CmdArgs.push_back("-lgcc_s");
I would prefer this part be a separate patch.
> diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp
> index e90cf8d..35c891a 100644
> --- a/lib/Frontend/CompilerInvocation.cpp
> +++ b/lib/Frontend/CompilerInvocation.cpp
> @@ -942,6 +942,7 @@ static void ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
> Opts.VerifyModule = !Args.hasArg(OPT_disable_llvm_verifier);
>
> Opts.InstrumentFunctions = Args.hasArg(OPT_finstrument_functions);
> + Opts.GPROFInstrument = Args.hasArg(OPT_pg);
>
> if (Arg *A = Args.getLastArg(OPT_fobjc_dispatch_method_EQ)) {
> llvm::StringRef Name = A->getValue(Args);
--
Thanks,
- Daniel
>
> It introduces GPROFInstrument CodeGenOption and sets it when -pg
> is specified. The FreeBSD ::Link() method was adjusted to link
> the correct libraries when -pg is specified. Finally, CodeGen
> was modified to emit calls to "mcount" (the actual name differs
> between arch/OS tuples) at the start of the function body.
>
> This patch is quite FreeBSD specific (the driver modification
> and the mcount name selection) but adding other OSes should be
> trivial once this is commited.
>
> Please review!
>
> roman
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
More information about the cfe-commits
mailing list