[PATCH] D91425: [DebugInfo] Skip dbg.value+derefs in RemoveRedundantDbgInstrs forward scan [3/3]

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.





================
Comment at: llvm/test/DebugInfo/Generic/dont-remove-redundant-dbg-derefs.ll:29
+  store i32 %param, i32* %param.addr, align 4
+  call void @llvm.dbg.value(metadata i32* %param.addr, metadata !12, metadata !DIExpression(DW_OP_deref)), !dbg !13
+  call void @use(i32* nonnull %param.addr), !dbg !18
----------------
Orlando wrote:
> bjope wrote:
> > bjope wrote:
> > > 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?
> > 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).
> > 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.
> 
> > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91425/new/

https://reviews.llvm.org/D91425