I have no problem with option (3).
As I had noted earlier, once you modify the pass so that the debug instruction doesn't interfere with the code motion, the associated debug instructions have to move with the real instruction. In that respect, the patch is doing the right thing.
@aprantl Thanks for example.
Moving DBG_VALUE instructions along with the instructions that define the associated values is the right thing to do.
That's an interesting question. I could see both sides of the argument:
Just pinging this. @bjope has provided great feedback (thanks!), but I am wondering if anyone else thinks this patch is useful, or if I should abandon it in favor of some other solution?
Thanks, this is looking pretty good now. I just have a few more small details, mostly to make it easier to read and understand what's going on.
@aprantl do you have any more concerns or is this ready for commit?
Thu, Jun 14
An assertion introduced by this patch is failing on a build bot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/25013
Wed, Jun 13
Privatize the flags for tracking MD5 usage.
Simplify bool updates.
Still works for me, but please add a comment explaining why getStoreSize is used here and what that means.
That's a great explanation, thank you!
After some more testing I discovered a case when the added assert in
void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII, PHINode *APN, DIBuilder &Builder)
Tue, Jun 12
@bjope , Thanks for your review and example! I agree that any changes to improve performSink/collectDebugValues should probably be in another patch.
Few more small nitpicks inline but otherwise LGTM.
Basically looks good to me now, as I think that getting rid of the debug-invariant problem is important.
When it comes to the sinking of DBG_VALUE instructions I believe that will be as good as the pre-RA version of MachineSinking. I think that could be enough to land this (as getting rid of the debug-invariance probably is more important).
More updates after review.
Mon, Jun 11
So llvm-mc performs a literal translation by default and with -g it generates debug info for the assembler input? Seems reasonable.
Thanks for the review @bjope!
Updated after comments from aprantl.