[PATCH] D73534: [WIP][DebugInfo] Enable the debug entry values feature by default

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 2 inline comments as done.
djtodoro added a comment.

In D73534#1845277 <https://reviews.llvm.org/D73534#1845277>, @dblaikie wrote:

> Do you have size measurements for the impact of this change (on, say, a self-host optimized build of clang)?


I will share these.

> & I think it's probably best to just change the default value of the flag - rather than changing the default and removing the flag. Giving folks some time with an escape hatch might be important.

I agree, thanks for the point!

> Is this patch intended for commit - or as a preview/working area? It seems it's got a bunch of things that could/should be done in independent commits (like the "Fix the assertions regarding updating the CallSiteInfo from the MF" parts, for instance - perhaps that was what was raised by another reviewer earlier).

That's why I've marked it as a 'WIP'. I am working on splitting the changes into reasonable parts.



================
Comment at: llvm/include/llvm/Target/TargetMachine.h:240-242
+  void setSupportsDebugEntryValues(bool Enable) {
+    Options.SupportsDebugEntryValues = Enable;
+  }
----------------
dblaikie wrote:
> Why is this a target machine feature? What aspect of debug entry values is target specific? Is there something about their architectures that makes it unrepresentable? (that seems drastic given the generality of the feature)
> 
> I'm just a bit hesitant to introduce target divergence if it can reasonably be avoided - it'll make debug info comparisons, cross platform work, etc, more difficult.
Since the debug entry values feature relies on the `CallSiteInfo` (that holds info about arguments and their transferring registers) which is target dependent, the whole feature is target dependent. The `CallSiteInfo` is being created during the `ISel` phase, when it performs the call lowering, which is also target dependent. Perhaps renaming the `SupportsDebugEntryValues` into something like `SupportsCallSiteInfo` would make sense here?

The idea of introducing the `CallSiteInfo` was that it could be used by some other tools or parts of the `LLVM` project. There was a discussion about that on the llvm-dev.

>I'm just a bit hesitant to introduce target divergence if it can reasonably be avoided - it'll make debug info comparisons, cross platform work, etc, more difficult.
Maybe an option for disabling the feature will be useful too for that purpose?


================
Comment at: llvm/test/DebugInfo/MIR/Hexagon/dbgcall-site-instr-before-bundled-call.mir:1
-# RUN: llc -mtriple hexagon -debug-entry-values -start-after=machineverifier -filetype=obj %s -o - | llvm-dwarfdump - | FileCheck %s
+# We do not support the call site info for the target now.
+# UNSUPPORTED: true
----------------
dstenb wrote:
> Is there any clean way we could force call site information, e.g. via a llc flag, for these now-unsupported tests? I guess there might be quite a bit of work of getting call site information up and running on these targets, which AFAIK no one is looking into at the moment, so I wonder if these tests risk laying unsupported here for a while?
> 
> (Since I added these Hexagon/Sparc/SystemZ tests, I just want to mention that I wrote them as upstream reproducers for issues I saw with our downstream DSP target. I currently do not have any plans on working on enabling call site information for any of those upstream targets.)
>Is there any clean way we could force call site information, e.g. via a llc flag, for these now-unsupported tests?
I see.. I do not now for any other way. Maybe we should keep the option, and change the default value only.

>I guess there might be quite a bit of work of getting call site information up and running on these targets, which AFAIK no one is looking into at the moment, so I wonder if these tests risk laying unsupported here for a while?
I think so. But we should keep the implementation up-to-date even for the targets that missing support for the `CallSiteInfo` production.


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

https://reviews.llvm.org/D73534