Implement -finstrument-functions. #1845

LemonBoy · 2016-10-19T22:37:01Z

Closes #1839.

dnadlinger · 2016-10-19T23:09:35Z

driver/cl_options.cpp

@@ -455,6 +455,11 @@ cl::opt<std::string> usefileInstrProf(
    cl::ValueRequired);
 #endif

+cl::opt<bool, true> instrumentFunctions(
+    "finstrument-functions",
+    cl::desc("Instrument function entry and exit with profiling calls"),


Maybe say "GCC-compatible profiling calls" to be clear?

That's what GCC says in the --help, verbatim, and Clang also says Generate calls to instrument function entry and exit, so I think that there's no need to further clarify the description.

Adding "GCC" may help, because you;d have to link with a GCC-compatible runtime I guess? (On Windows, it would help folks figure out what's wrong?)

Indeed, the stubbed out functions are actually defined in libc.so and it didn't occur to me that the MSVC libc doesn't have those (doesn't it ?).

dnadlinger · 2016-10-19T23:10:43Z

gen/tollvm.cpp

+    LLType *VoidTy = LLType::getVoidTy(gIR->context());
+    LLType *Tys[] = {VoidPtrTy, VoidPtrTy};
+    auto fty = LLFunctionType::get(VoidTy, Tys, false);
+    fn = LLFunction::Create(fty, LLGlobalValue::ExternalLinkage, fnName,


Are there any LLVM attributes that could allow more optimisations here, or do we need to allow arbitrary hook functions?

We can't make any assumption about what's done in the functions, better play it safe.

I think we usually put the definition of these "runtime" functions in runtime.cpp.
Attributes you may think about are "nocapture" "readonly" or something like that. Runtime.cpp has a bunch of that.
I had trouble finding where these cyg_profile functions are defined, did you find a link to an implementation?

Are these profiling functions ever used in a debugging context? What I mean is: is it OK if the compiler reorders things, e.g.:

void foo() { __cyg_profile_func_start(...); smth(); __cyg_profile_func_exit(...); }

optimized (for whatever reason) to:

void foo() { __cyg_profile_func_start(...); __cyg_profile_func_exit(...); smth(); }

To prevent that, perhaps an attribute is needed on the function declarations (or perhaps the absence of attributes means that LLVM assumes the worst).

I wasn't 100% sure about where to put this, I can move it to runtime.cpp if that's deemed a best place. I'll also have a look at what those attributes are for and which ones are applicable.

WRT the definition of the symbols, LD_DEBUG shows
binding file ./test [0] to /usr/lib/libc.so.6 [0]: normal symbol__cyg_profile_func_enter' [GLIBC_2.2.5] so it'slibc` that provides the stubs...we might provide such stubs in the druntime, no ?

I think in runtime.cpp is best: for example, you forgot to explicitly set the C-calling convention, and there may be other things that end up forgotten. The createFwdDecl function was invented for this ;-)

JohanEngelen · 2016-10-20T12:29:24Z

tests/codegen/instrumentation.d

+
+pragma(LDC_profile_instr, false)
+void fun1 () {
+  // CHECK: ret void


this doesn't check for the absense of __cyg_profile calls

My bad, I'm still convinced that lit's smarter than it really is :)

JohanEngelen · 2016-10-20T12:32:59Z

tests/codegen/instrumentation.d

+// RUN: %ldc -c -output-ll -finstrument-functions -of=%t.ll %s && FileCheck %s < %t.ll
+
+void fun0 () {
+  // CHECK: %1 = call i8* @llvm.returnaddress(i32 1)


You could add some simple code to the function to test that the profiling function calls enclose any "body" code.

Also you could add a function with more than one exit path, and a function with an exception throwing exit path, or a non-trivial destructor call. Again to check where the profiling calls are placed.

JohanEngelen · 2016-10-20T13:13:50Z

Great stuff btw :-)

JohanEngelen · 2016-10-20T16:20:45Z

ddmd/globals.h

@@ -202,6 +202,7 @@ struct Param
    OUTPUTFLAG output_bc;
    OUTPUTFLAG output_s;
    OUTPUTFLAG output_o;
+    bool instrumentFunctions;


this must be added to the struct in globals.d too. May fix the CI tester failures.

(it'd be so nice if there was a tool to check types that are declared in both C++ and D!)

actually: better to not add it to globals.param, and to add it to cl_options.h. (similar to linkonceTemplates)

JohanEngelen · 2016-10-20T16:39:13Z

tests/codegen/instrumentation.d

+int fun1 (int x) {
+  // CHECK: %x = alloca i32, align 4
+  // CHECK-NEXT: store i32 %x_arg, i32* %x
+  // CHECK-NEXT: ret i32 42


to check the absence of _cyg_profile_func{enter,exit}, you should just do a // CHECK-NOT: __cyg_profile_func_.

Because you only want to check that for this one particular function, you should add CHECK-LABEL checks for each function, such that the file is "sectioned".

JohanEngelen · 2016-10-20T16:40:54Z

tests/codegen/instrumentation.d

+  if (x < 10)
+    // CHECK: if:
+    // CHECK-NEXT: %{{.}} = call i8* @llvm.returnaddress(i32 1)
+    // CHECK-NEXT: call void @__cyg_profile_func_exit


how about

// CHECK: call void @__cyg_profile_func_exit // CHECK-NEXT: ret

here, and at the end of the functions?

JohanEngelen

Good stuff. Please look at the two comments.

JohanEngelen · 2016-10-28T15:40:46Z

gen/functions.cpp

@@ -973,6 +973,9 @@ void DtoDefineFunction(FuncDeclaration *fd, bool linkageAvailableExternally) {
  // debug info - after all allocas, but before any llvm.dbg.declare etc
  gIR->DBuilder.EmitFuncStart(fd);

+  if (opts::instrumentFunctions && fd->emitInstrumentation)
+    emitInstrumentationFn(false);


This is a little nitpicky / subjective, but how about adding two helper functions emitInstrumentationFuncStart(FuncDeclaration *fd) and emitInstrumentationFuncEnd in runtime.h?
Note that other emitSomething functions do the check whether to actually do something inside, instead of outside like you do here.

inline void emitInstrumentationFuncStart(FuncDeclaration *fd) { if (opts::instrumentFunctions && fd->emitInstrumentation) emitInstrumentationFn(false); }

JohanEngelen · 2016-10-28T15:41:18Z

gen/runtime.cpp

+  // void __cyg_profile_func_exit(void *callee, void *caller)
+  createFwdDecl(LINKc, voidTy,
+                {"__cyg_profile_func_exit", "__cyg_profile_func_enter"},
+                {voidPtrTy, voidPtrTy});


as we discussed, please add the nounwind attribute here.

Closes ldc-developers#1839.

JohanEngelen · 2019-01-21T16:54:36Z

#1961

LemonBoy force-pushed the instr branch from 1a617d1 to a80d002 Compare October 19, 2016 22:42

dnadlinger approved these changes Oct 19, 2016

View reviewed changes

JohanEngelen reviewed Oct 20, 2016

View reviewed changes

LemonBoy force-pushed the instr branch 2 times, most recently from 43fa944 to ce07808 Compare October 20, 2016 13:48

JohanEngelen reviewed Oct 20, 2016

View reviewed changes

LemonBoy force-pushed the instr branch 3 times, most recently from e0124bf to 01c10c7 Compare October 20, 2016 21:22

JohanEngelen approved these changes Oct 28, 2016

View reviewed changes

LemonBoy force-pushed the instr branch 2 times, most recently from a283b4c to b41a3f8 Compare November 3, 2016 22:32

Implement -finstrument-functions.

596fc82

Closes ldc-developers#1839.

LemonBoy force-pushed the instr branch from b41a3f8 to 596fc82 Compare November 6, 2016 22:17

kinke mentioned this pull request Nov 7, 2016

A new beta #1862

Closed

kinke merged commit e154826 into ldc-developers:master Nov 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement -finstrument-functions. #1845

Implement -finstrument-functions. #1845

LemonBoy commented Oct 19, 2016

dnadlinger Oct 19, 2016

LemonBoy Oct 20, 2016

JohanEngelen Oct 20, 2016

LemonBoy Oct 20, 2016

dnadlinger Oct 19, 2016

LemonBoy Oct 20, 2016

JohanEngelen Oct 20, 2016

JohanEngelen Oct 20, 2016

LemonBoy Oct 20, 2016

JohanEngelen Oct 20, 2016

JohanEngelen Oct 20, 2016

LemonBoy Oct 20, 2016

JohanEngelen Oct 20, 2016

JohanEngelen commented Oct 20, 2016

JohanEngelen Oct 20, 2016

JohanEngelen Oct 20, 2016 •

edited

Loading

JohanEngelen Oct 20, 2016

JohanEngelen Oct 20, 2016

JohanEngelen left a comment

JohanEngelen Oct 28, 2016

JohanEngelen Oct 28, 2016

JohanEngelen commented Jan 21, 2019

Implement -finstrument-functions. #1845

Implement -finstrument-functions. #1845

Conversation

LemonBoy commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Oct 20, 2016

Choose a reason for hiding this comment

JohanEngelen Oct 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Jan 21, 2019

JohanEngelen Oct 20, 2016 •

edited

Loading