[PATCH] D40413: [CodeExtractor] Add debug locations for new call and branch instrs.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for having a look! I'll address the assertion before committing tomorrow and also think a bit about the PGO case.



================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:1050
+      }
+    assert(BranchI->getDebugLoc() && "Could not find debug location in header");
+  }
----------------
aprantl wrote:
> This assertion will fail if you haved a nodebug function inlined into a function with debuginfo.
Ah yes, I will look into how the inliner deals with that case and handle it appropriately.


================
Comment at: test/Transforms/CodeExtractor/PartialInlineDebug.ll:30
+; CHECK-LABEL: define internal void @callee.1_if.then
+; CHECK: br label %if.then, !dbg ![[DBG3:[0-9]+]]
+; : ![[DBG1]] = !DILocation(line: 10, column: 7,
----------------
danielcdh wrote:
> I'm wondering if we inline callee.1_if.then back into caller, what would the inline stack be for this instruction. Looks like it will be:
> 
> test.c:10 (callee.1_if.then)
> test.c:10 (callee)
> test.c:5 (caller)
> 
> But from the PGO point of view, the correct inline stack should be:
> 
> test.c:10 (callee)
> test.c:5 (caller)
> 
> Not sure if PGO-desired inline stack is possible with partial inlining with or without this patch.
> 
> Maybe we should just disable partial inlining when the binary is to be used to collect SamplePGO profile, unless it's designed for flattened profile. +wmi who's evaluating flattened profile.
Great, thanks for having a look! Currently, partial inlining is disabled by default, but it sounds like we have to handle this issue before enabling it or disable it with PGO for now.

Is the problem for PGO with splitting callee into 2 functions the following: all samples would be (incorrectly) attributed to `callee` and not split between `callee` and `callee.1_if.then`?


https://reviews.llvm.org/D40413