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

[David Blaikie via llvm-commits]

dblaikie accepted this revision.
dblaikie added a reviewer: dblaikie.
dblaikie added a comment.
This revision is now accepted and ready to land.

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.



================
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;
----------------
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? 


================
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;
----------------
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.


https://reviews.llvm.org/D24180