Page MenuHomePhabricator

[CallSiteInfo] Enable the call site info only for -g + optimizations
ClosedPublic

Authored by djtodoro on Feb 26 2020, 6:49 AM.

Details

Summary

Emit call site info only in the case of '-g' + 'O>0' level.

Diff Detail

Event Timeline

djtodoro created this revision.Feb 26 2020, 6:49 AM
djtodoro updated this revision to Diff 246945.Feb 27 2020, 7:50 AM
djtodoro added reviewers: vsk, aprantl, dstenb.
djtodoro added a project: debug-info.
djtodoro added subscribers: petarj, asowda, ntesic.

-Rebasing

As I mentioned on D74904, I think it makes sense to avoiding production of call site info in the case of non-debug builds, since it is being used only for Debug Entry Values feature.
The Call Site Info potentially could be used by some other LLVM modules, but if we find such case, we can enable it for that purpose as well.

The separate option makes sense to me, since the Call Site Info is conceptually different thing, and the Debug Entry Values feature just uses the Call Site Info utility.

If this seems reasonable, I will rebase the re-landing of D73534 on top of this.

One downside with this is that if you compile the compilation unit containing the caller with -O0, and the compilation unit containing the callee with optimizations enabled, it will not be possible to evaluate the entry values in the callee. However, GCC (8.3.0) seems to behave in the same way; at least for the small examples I tried.

llvm/include/llvm/Target/TargetOptions.h
261

Nit: optimize -> optimized

266

What do you think about keeping these as two separate things, and query one of EmitCallSiteInfo / EnableDebugEntryValues at each use? I think that it might make it easier to understand the code if they are treated as separate throughout the compiler rather than being merged here.

Seems reasonable.

clang/include/clang/Basic/CodeGenOptions.def
68

I think this comment should go into the help text for the command line parameter instead?

One downside with this is that if you compile the compilation unit containing the caller with -O0, and the compilation unit containing the callee with optimizations enabled, it will not be possible to evaluate the entry values in the callee.

Yes, but this will restrict only call-site-info (callSites: MF attribute) production. The DWARF (tags for call_sites and call_site_params) was already controlled by the DISubprogram flag (all-calls-described), so I think with or without this that will be the same?

djtodoro marked an inline comment as done.Feb 27 2020, 9:05 AM
djtodoro added inline comments.
clang/include/clang/Basic/CodeGenOptions.def
68

That sounds good to me. Thanks!

djtodoro marked an inline comment as done.Feb 27 2020, 9:06 AM
djtodoro added inline comments.
llvm/include/llvm/Target/TargetOptions.h
266

It is good idea actually! thanks!

vsk added inline comments.Feb 27 2020, 1:35 PM
clang/lib/Frontend/CompilerInvocation.cpp
1437

Is this a replacement for the EnableDebugEntryValues codegenopt in clang?

djtodoro marked an inline comment as done.Feb 28 2020, 6:45 AM
djtodoro added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1437

It is not a full replacement, since it applies only to the Call Site Info. It could end up acting similar to EnableDebugEntryValues, but it is conceptually different.

But it should go above where the EnableDebugEntryValues was set.

djtodoro updated this revision to Diff 247255.Feb 28 2020, 6:47 AM

-Addressing comments

TODO: Rebase relanding of the D73534 on top of this.

djtodoro marked an inline comment as done.Feb 28 2020, 6:53 AM
djtodoro added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1437

But I would keep the EnableDebugEntryValue Target option as well, if that was the question.

vsk added inline comments.Feb 28 2020, 10:29 AM
clang/lib/Frontend/CompilerInvocation.cpp
793

I don't understand how Opts.EnableDebugEntryValues and Opts.EmitCallSiteInfo are different. What is the second option supposed to control, that the first option does not? I thought we could already suppress entry values in llvm at -O0 + -g.

Is EmitCallSiteInfo = false supposed to disable TAG_call_site emission?

djtodoro marked an inline comment as done.Mar 2 2020, 12:27 AM
djtodoro added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
793

I'll try to describe the difference between Call Site Info and entry values.

Entry values (call-site-params and dbg-values with dw_op_entry_val) are features from debug info. The call-site-params are extension of the call-site (TAG_call_site) debug info. Since the emission of the TAG_call_site was already restricted to -O>0 + -g in the constructCallSiteEntryDIEs() (there is a code checking the existence of the AllCallsDescribed), that implies the collectCallSiteParameters() will be called for that combination only (since it is being called in the constructCallSiteEntryDIEs()). Further on, the collectCallSiteParameters() will call the describeLoadedValue(), which is target-dependent method, and it is implemented only for x86, arm and aarch64 targets at the moment, so we need to restrict the emission of call-site-params only to the targets.

The Call Site Info is utility helping us implementing the entry values. We did not have the information about arguments forwarding (necessary for implementing the describeLoadedValue() by searching for the values loaded into the forwarding registers), so the conclusion was to add such info as MF attribute (callSites:). In addition, there were thoughts the attribute could be used by other places in LLVM, but currently it is being used only by Debug Entry Values feature. My understanding is the MF attribute is different thing to the Debug Entry Values, and we can not restrict the production of such attribute by looking at the Debug Info Metadata (for example by looking at the AllCallsDescribed flag), so that is why I added the additional flag.

Is EmitCallSiteInfo = false supposed to disable TAG_call_site emission?

No, it should restrict only the callSites MF attribute.

The rebasing of the D73534 would look like (generated with -U999999):

aprantl added inline comments.Mar 5 2020, 8:46 AM
llvm/include/llvm/CodeGen/CommandFlags.inc
280

Let's not use a wording like Currently, it will start looking awkward soon.

"Emit call site debug information, if debug information is enabled."

282

Again, this text can't be in LLVM since it is Clang's decision when to enable this. (At least I'm assuming that it is Clang :-). A different fronted could conceivably make different choices.

djtodoro marked an inline comment as done.Mar 6 2020, 1:47 AM

@aprantl Thanks!

llvm/include/llvm/CodeGen/CommandFlags.inc
282

Yes.. some frontends could have benefits of this even with -O0.. I am moving that into clang's part :)

djtodoro updated this revision to Diff 248667.Mar 6 2020, 1:50 AM

-Addressing comments

aprantl accepted this revision.Mar 6 2020, 8:45 AM
This revision is now accepted and ready to land.Mar 6 2020, 8:45 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2020, 4:16 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vsk added a comment.Mar 17 2020, 9:53 AM

Sorry for the delay here.

clang/lib/Frontend/CompilerInvocation.cpp
793

Thanks for explaining, this sgtm.