[PATCH] D68209: [LiveDebugValues] Introduce entry values of unmodified params

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked an inline comment as done.
djtodoro added a comment.

Hi Jeremy,

Thanks a lot for diving into this!

> However, the test case demonstrates something I've found odd in LLVM instruction selection: it creates _two_ DBG_VALUEs for arguments. The code that does it is here [1], dates back to about 2010, and sounds like it tries to propagate variable locations in a way that is now LiveDebugValues domain. If it didn't create an extra DBG_VALUE for every argument, we might not need special handling for extra DBG_VALUEs here. Removing the code in [1] makes very little difference to variable-location coverage statistics, but does trip 12 tests.

Hmm... I see. I agree that that should be the `LiveDebugValues` job..
Initially when I did the investigation, I have not used the D67393 <https://reviews.llvm.org/D67393>, but I see that it should remove large portion of the cases we care about here.

> I don't have any specific feedback on this patch, but I get the feeling that it might not be necessary if we eliminated [1];

Anyhow, I think even we decide to remove the code from [1], we would need one part of this patch, to avoid some unwanted entry values generated. But, the code (and the analysis) will be away more simpler than the current one.

> (Sorry for driving this review in an unrelated direction, but it'd be nice if we could design-out this problem).

I really appreciate the comments, since resolving the issue from [1] will simplify this a lot. :)



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1449
         !DebugEntryVals.count(MI.getDebugVariable()) &&
         MI.getDebugExpression()->getNumElements() == 0)
+      DebugEntryVals[MI.getDebugVariable()] = EntryValue{&MI, nullptr};
----------------
dstenb wrote:
> jmorse wrote:
> > Note that this hunk didn't apply cleanly for me; on master this line (1319/1449) reads
> > 
> >     !MI.getDebugExpression()->isFragment())
> That is from D66746. That patch has been postponed a bit as I've been working on other patches, but I'll pick up that again.
I forgot to add a note that this relies on the D66746 as well. Sorry for that, I will update the diff.


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

https://reviews.llvm.org/D68209