See comment 3 on PR47946.
InlineLowerDbgDeclare looks for dbg.value+derefs which have been inserted before
call instructions by LowerDbgDeclare. However, without this patch these
intrinsics are eligable for removal by RemoveRedundantDbgInstr.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| llvm/lib/Transforms/Utils/BasicBlockUtils.cpp | ||
|---|---|---|
| 423–425 | Not sure I understand exactly how/if this motivates why the dbg.value should be kept. If we first say that the variable is at address X, and then later say that the variable is at address X, the second statement seem redundant to me. | |
| llvm/test/DebugInfo/Generic/dont-remove-redundant-dbg-derefs.ll | ||
| 30 | So if you do this ten times in a row (couldn't that be the case if for example unrolling a loop fully?) you would keep all of them as well. An alternative would be to filter out all entries with a deref from the VariableMap when finding a call. Or is it still important that the dbg.value with a deref is just before the call? Then, how is it guaranteed that no other pass is inserting something between the dbg.value and the call in such sitautions? | |
| llvm/test/DebugInfo/Generic/dont-remove-redundant-dbg-derefs.ll | ||
|---|---|---|
| 30 | Looking at patch 2/3 it seems like InlineLowerDbgDeclare only handle the dbg.value that are adjacent to the call. One could use an intermediate MaybeToBeRemoved map that is cleaned when finding a call, or appended to ToBeRemoved (and cleaned) when finding a non-dbg-intrinsic during the forward scan. That would pinpoint the algorithm to just keep dbg.value+deref just before a call. But maybe not worth the fuzz if these otherwise redundant dbg.value+deref instructions are rare. Still, it might be nice to for example duplicate the dbg.value here and on line 31 to see that the backward scan still eliminates redundant dbg.value+deref when they are adjacent to each other (non non-dbg-intrinsic in between). | |
Comment Actions
Thank you @bjope for taking a look at this.
| llvm/test/DebugInfo/Generic/dont-remove-redundant-dbg-derefs.ll | ||
|---|---|---|
| 30 |
Unfortunately there are no guarantees. I don't like this but I'm not sure what else we can reasonably do within the current debug-info framework.
Yeah that's right.
In practice I don't think there are any problems with this suggestion, but based on @jeremy's comment here https://bugs.llvm.org/show_bug.cgi?id=47946#c4 I'm not sure whether we should remove any dbg.value+derefs in the forward scan. What do you think?
SGTM, I will update the test. | |
Comment Actions
| llvm/test/DebugInfo/Generic/dont-remove-redundant-dbg-derefs.ll | ||
|---|---|---|
| 30 |
I don't see it as incorrect to remove them. We currently only support that a variable is described by one location. So given a dbg.value or dbg.addr that says that binds a variable to a location we see it as that binding is valid until we find another dbg.value or dbg.addr that maps the variable to something different. Repeating the same dbg.value over and over again is not really adding any information (it just makes IR output longer and makes it more time consuming to iterate over the IR when not dealing with dbg-info). But if this patch (leaving some dbg.value+deref) just before a call currently improve the debug experience I won't stop this patch. Just make it clear that the solution isn't bullet-proof. E.g. in case some pass inserts an instruction just before the call but after the dbg.value then the dbg.value may be removed. So it is a simple (cheap) solution to improve things in many situations, but generally speaking the problem could remain in some situations. | |
Comment Actions
Address feedback from @bjope:
+ Change comment for dbg.value+derefs in ShouldRemove lambda.
+ Update the test to check that the backward scan removes adjacent duplicate dbg.value+derefs still.