[PATCH] D70350: [DWARF] Allow cross-CU references of subprogram definitions

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/test/DebugInfo/X86/lto-cross-cu-call-origin-ref.ll:5-7
+; This test checks that cross-CU references to subprograms within call site
+; tags are well-formed. There are 5 cases checked in this test. Each set of
+; checks is numbered and has a brief summary.
----------------
dblaikie wrote:
> This is specifically only for subprogram definitions, right? (you can have cross-CU references for subprogram declarations - but that's already handled without this patch?) so probably add "definitions in there ("references to subprogram >definitions< within call site tags are well-formed"...)
Yes, I'll add this in.


================
Comment at: llvm/test/DebugInfo/X86/lto-cross-cu-call-origin-ref.ll:10-11
+; Instructions to regenerate the IR:
+; clang -O2 -g -emit-llvm -o a.bc -c a.c
+; clang -O2 -g -emit-llvm -o b.bc -c b.c
+; llvm-link -o linked.bc a.bc b.bc
----------------
dblaikie wrote:
> You need -O2 to get the call site debug info? Is there any way to ask for that specifically without enabling optimizations? Might make it clearer (not like the optimizations can do anything given the optnone constraints, but might be a bit simpler if the optnone could be removed and the -ON could be removed too).
Currently there is no way to force call site tag emission without optimizations (see `CGDebugInfo::getCallSiteRelatedAttrs`), although it might be simpler to use -O1 here.


================
Comment at: llvm/test/DebugInfo/X86/lto-cross-cu-call-origin-ref.ll:13
+; llvm-link -o linked.bc a.bc b.bc
+; opt -O1 linked.bc -o merged.bc
+
----------------
dblaikie wrote:
> Should be able to do this at -O0 (that'd still run the always_inliner)?
Not quite. AlwaysInliner does run at -O0, but because the frontend isn't generating IR in -flto mode here, calls to an external (always_inline) function do not get the "always_inline" attribute. So the inliner declines to inline with:

```
Inliner visiting SCC: always_inline_helper_in_a_that_calls_foo: 1 call sites.
    NOT Inlining (cost=never): no alwaysinline attribute, Call:   call fastcc void @foo.2(), !dbg !20
```


================
Comment at: llvm/test/DebugInfo/X86/lto-cross-cu-call-origin-ref.ll:56
+
+; CHECK: 0x{{0+}}[[NOINLINE_FUNC_IN_A:.*]]: DW_TAG_subprogram
+; CHECK:   DW_AT_name ("noinline_func_in_a")
----------------
dblaikie wrote:
> You can probably remove the "0x{{0+}}" from these lines (& let those parts be included in the NOINLINE_FUNC_IN_A match implicitly (eg, write this line as:
> 
>   ; CHECK: [[NOINLINE_FUNC_IN_A:.*]]: DW_TAG_subprogram
> 
> & the usage would change from this:
> 
>   ; CHECK-NEXT: DW_AT_call_origin (0x{{0+}}[[NOINLINE_FUNC_IN_A]])
> 
> to this:
> 
>   ; CHECK-NEXT: DW_AT_call_origin ([[NOINLINE_FUNC_IN_A]])
> 
> Similarly with the other matches.
This can be done in some places, but I think the net effect is to confuse things a bit, as the `{{0+}}` can't be left out of the match in all cases. E.g. it cannot be left out when the matched DIE is referenced by both CU's: in this case, there cannot be a `{{0+}}` in the check of the same-CU ref, as this would match too many zeros. Better to use the same check/match pattern everywhere, imo, instead of requiring the reader to figure out why the patterns change.


================
Comment at: llvm/test/DebugInfo/X86/lto-cross-cu-call-origin-ref.ll:84-90
+; CHECK: DW_TAG_subprogram
+; CHECK:   DW_AT_name ("noinline_func_in_a")
+; CHECK:   DW_AT_declaration (true)
+
+; CHECK: DW_TAG_subprogram
+; CHECK:   DW_AT_name ("always_inline_helper_in_a_that_calls_foo")
+; CHECK:   DW_AT_declaration (true)
----------------
dblaikie wrote:
> These two look out of place - shouldn't they be referencing the DIEs in A's CU rather than emitting declarations here in B? (perhaps this is a separate bug to be fixed in another change, I don't know)
I think this is a separate bug, in which DwarfDebug::getOrCreateCompileUnit (over-)eagerly emits declaration DIEs. IIUC it doesn't have to emit these declarations in the LTO case, or when call site info has been disabled.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70350/new/

https://reviews.llvm.org/D70350