[PATCH] D83895: [DebugInfo] Process DBG_VALUE_LIST in LiveDebugVariables

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

In D83895#2225768 <https://reviews.llvm.org/D83895#2225768>, @Orlando wrote:

> I'm hoping you can clarify something about `addDefsFromCopies` for me. IIUC without the patch we are adding defs from copies at kill points if possible. And with the patch a def is only added on the _first_ kill. Is that right?

That's what it says on the tin, but after actually examining the code that calls `addDefsFromCopies` it appears that we never passed more than one kill point before this change, hence the slight rewrite. More specifically, `addDefsFromCopies` is only called in one place; in that place, the `Kills` vector is declared locally and then passed to `extendDef` immediately before that call, and `extendDef` always returns immediately after adding a single item to `Kills`, meaning that it's really more of an optional than a vector.



================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:103
 public:
-  DbgVariableValue(unsigned LocNo, bool WasIndirect,
-                   const DIExpression &Expression)
-      : LocNo(LocNo), WasIndirect(WasIndirect), Expression(&Expression) {
-    assert(getLocNo() == LocNo && "location truncation");
+  DbgVariableValue(ArrayRef<unsigned> NewLocs, bool WasIndirect, bool WasList,
+                   const DIExpression &Expr)
----------------
Orlando wrote:
> It looks like this class was trying to be small on purpose, and it is now relatively much larger. I don't know if it will make a significant impact or not, have you managed to run any kind of performance tests with this patch yet?
I did notice this when changing the class, and I share the concerns. Performance testing this implementation is in-progress, but I imagine that if the size is a concern then it should be sufficient to shrink the size of the array. In most cases we only actually need one element, so compared to the old class size size of 12 bytes with 64-bit pointers, this would be 21 bytes - an increase, but hopefully not drastic. If this turns out to still cause a major performance hit though, then it will probably need a more drastic redesign. I'm optimistic here however, as LiveDebugVariables is not usually a performance bottleneck (compared to LiveDebugValues).


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1516-1517
 
-    LLVM_DEBUG(dbgs() << "\t[" << Start << ';' << Stop
-                      << "):" << DbgValue.getLocNo());
+    LLVM_DEBUG(auto &dbg = dbgs(); dbg << "\t[" << Start << ';' << Stop << "):";
+               DbgValue.printLocNos(dbg));
     MachineFunction::iterator MBB = LIS.getMBBFromIndex(Start)->getIterator();
----------------
Orlando wrote:
> Is it not possible to do this instead?
> ```
>     LLVM_DEBUG(dbgs() << "\t[" << Start << ';' << Stop << "):";
>                DbgValue.printLocNos(dbgs()));
> ```
When the `ostream` returned by `dbgs()` falls out of scope, it adds a banner (with a newline) and flushes output; we only want this to happen once for the entire print statement here, so we need to assign it to a local variable so that we can use the same object between statements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83895