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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/include/llvm/Target/TargetMachine.h:240-242
+  void setSupportsDebugEntryValues(bool Enable) {
+    Options.SupportsDebugEntryValues = Enable;
+  }
----------------
djtodoro wrote:
> 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?
I think this flag logically makes sense as part of TargetMachine, but the name seems strange. I think all targets support "call site info" in the sense of DW_TAG_call_site. I think this should be renamed to `SupportsDebugEntryValues`.


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:875
+  // well.
+  bool SupportsDebugEntryValues = Target.Options.SupportsCallSiteInfo |
+                                  Target.Options.EnableDebugEntryValues;
----------------
Could you just add a `shouldEmitDebugEntryValues` predicate to TargetOptions that checks both flags, and use that everywhere? Also, please use `||` for logical ops.


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

https://reviews.llvm.org/D73534