[PATCH] D24180: Emit 'no line' information for interesting 'orphan' instructions

Paul Robinson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 29 14:50:16 PST 2016


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

In https://reviews.llvm.org/D24180#604826, @dblaikie wrote:

> Looks good (you could reduce some indentation in nthe newly introduced blocks if you like by using "if (!cond) return; work; return; " rather than "if (cond) { work; }" but leave it up to you if it feels better or not.


I did a little bit of that, it does help break out the cases better.  Thanks!



================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1041-1049
+    if (LastAsmLine != 0 &&
+        (UnknownLocations || PrevLabel ||
+         (PrevInstBB && PrevInstBB != MI->getParent()))) {
+      // Preserve the file number, if we can, to save space in the line table.
+      // Do not update PrevInstLoc, it remembers the last non-0 line.
+      // FIXME: Also preserve the column number, to save more space?
+      const MDNode *Scope = PrevInstLoc ? PrevInstLoc.getScope() : nullptr;
----------------
dblaikie wrote:
> I would've expected this conditional to be:
> 
>   if (UnknownLocations || (...))
> 
> (essentially UnknownLocations triggers this in all cases, otherwise we trigger it in the subset of cases you're trying to implement (at the beginning of BB's, etc))
> 
> I'm assuming recordSourceLine does the appropriate thing to avoid producing redundant entries (eg: calling it with 0 twice is a no-op) - so the reordering of the condition you've done here is a minor optimization (to avoid calling recordSourceLine(0..) when it was already called last anyway), not a functional change? 
Actually no, recordSourceLine does not suppress duplicates.  And because I don't set `PrevInstLoc = DL;` here anymore, I have to use LastAsmLine to determine whether I've already emitted a line-0 record.
But I think it does make sense to pull out
```
if (LastAsmLine == 0)
  return;
```
separately, it makes the commentary flow better and makes UnknownLocations etc condition easier to understand.




================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1046
+      // Do not update PrevInstLoc, it remembers the last non-0 line.
+      // FIXME: Also preserve the column number, to save more space?
+      const MDNode *Scope = PrevInstLoc ? PrevInstLoc.getScope() : nullptr;
----------------
dblaikie wrote:
> Yeah - we have column info off by default (& it does make the line table significantly larger) if I recall correctly. But would be nice to have this improvement to this change at some point.
Can do.


https://reviews.llvm.org/D24180





More information about the llvm-commits mailing list