[PATCH] D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink

Matt Davis via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 20 12:24:18 PDT 2018


mattd added inline comments.


================
Comment at: lib/CodeGen/MachineSink.cpp:764
+  // Merge or erase debug location to ensure consistent stepping in profilers
+  // and debuggers.
+  if (!SuccToSinkTo.empty() && InsertPos != SuccToSinkTo.end())
----------------
aprantl wrote:
> Can you add a sentence explaining *why* merging vs. erasing is the right thing to do in the respective cases?
I lifted this bit of the code from the original implementation and placed it into a routine to be shared by a few functions in this file (SinkInstruction and tryToSinkCopy).  Mainly, I wanted tryToSinkCopy to account for the movement of any adjacent debug values associated with the to-be-sunken copy, and the code was already there for that functionality, but in SinkInstruction.  We need that functionality for tryToSinkCopy too.

With that said, it seems to me that the intention here (to merge or erase), is a conservative approach. 
getMergedLocation, in this form, will only accept a location if both the sunken instruction and its destination have the same (non-discriminated) location, otherwise the location is erased.  I have heard word that getMergedLocation might change, but that's probably a separate discussion.  I think the comment should probably be updated as:
"Only preserve MI's location if it is the same as InsertPos.  This removes the location in cases
where MI is landing in a different, or empty, block from what the location is currently set to.  By conservatively erasing the debug location, we also eliminate the potential for reporting wrong line information when observing the sunken instruction via debug-info driven utilities."


https://reviews.llvm.org/D45637





More information about the llvm-commits mailing list