Page MenuHomePhabricator

[DebugInfo] Skip dbg.value+derefs in RemoveRedundantDbgInstrs forward scan [3/3]
Needs ReviewPublic

Authored by Orlando on Nov 13 2020, 7:17 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Orlando created this revision.Nov 13 2020, 7:17 AM
bjope added inline comments.Nov 18 2020, 6:18 AM
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.
(well I guess the backward scan would clean it up if they really are consecutive)

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?

bjope added inline comments.Nov 18 2020, 6:33 AM
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).

Thank you @bjope for taking a look at this.

llvm/test/DebugInfo/Generic/dont-remove-redundant-dbg-derefs.ll
30

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?

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.

Looking at patch 2/3 it seems like InlineLowerDbgDeclare only handle the dbg.value that are adjacent to the call.

Yeah that's right.

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.

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?

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)

SGTM, I will update the test.

bjope added a comment.Nov 20 2020, 7:01 AM
llvm/test/DebugInfo/Generic/dont-remove-redundant-dbg-derefs.ll
30

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.

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?

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.

Orlando updated this revision to Diff 316604.Thu, Jan 14, 2:51 AM

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.