[PATCH] D125693: [DebugInfo] Support types, imports and static locals declared in a lexical block (3/5)

[Ellis Hoag via Phabricator via cfe-commits]

ellis added inline comments.


================
Comment at: llvm/test/Instrumentation/InstrProfiling/debug-info-correlate.ll:23
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 12345678, i32 2, i32 0)
-  ret void
+  ret void, !dbg !17
 }
----------------
krisb wrote:
> ellis wrote:
> > krisb wrote:
> > > ellis wrote:
> > > > I asked the same question in D113741, but why is this test changed?
> > > Normally, we emit function-local entities iff a parent function has location information. This is done the same way for local variables, labels, parameters, imported entities, and, //now,// for static locals as well.
> > > Before this change static locals behaved more like global variables and get emitted doesn't matter its parent function. This patch makes them handled more like local variables.
> > > 
> > > I believe either the call or the 'ret' (or, likely, both) had had DILocation attached originally, but it has been removed to simplify the test.
> > I just checked and the `llvm.instrprof.increment` intrinsic does not have debug info attached. I think you're right that I removed debug info from the `ret` instruction to simplify the test.
> > 
> > This is a special case where a global and its debug info is synthesized.
> > https://github.com/llvm/llvm-project/blob/23c2bedfd93cfacc62009425c464e659a34e92e6/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp#L976-L1001
> > 
> > So I don't understand why this added debug info is necessary. Does the test fail otherwise?
> Right, the test would fail w/o the added DILocation, because `__profc_foo` variable (which is function-local 'global' technically) would not be emitted. 
> With this patch we emit function-local entities if and only if the parent function has LexicalScope defined (see createAndAddScopeChildren() and its call from constructSubprogramScopeDIE()), otherwise we skip emitting all function's children (including function-local 'globals').
Got it, thanks for explaining!

I'm ok with this because `llvm.instrprof.increment` should probably be next to some instruction with debug info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125693