[PATCH] D48676: [Local] replaceAllDbgUsesWith: Update debug values before RAUW

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In https://reviews.llvm.org/D48676#1153642, @vsk wrote:

> In https://reviews.llvm.org/D48676#1153467, @bjope wrote:
>
> > FYI: Looks like you have address my earlier comments. So I have no blocking remarks.
>
>
> Thanks for taking a look, I really appreciate your detailed feedback.
>
> > The addition of DomPoint / DT was interesting (not sure that I have figured out exactly which scenarios that are handled)
>
> The two scenarios are:
>
> 1. The replacement value is a Constant, will be inserted at a position which dominates the original instruction, or will be inserted immediately after the original instruction (which is deleted later). In this scenario there's no risk of introducing debug value use-before-def.
> 2. The original value dominates the position where the replacement value will be inserted. A good example of this is the 'alloca-cast-debuginfo.ll' test. In an intermediate step, InstCombine inserts a temporary cast of an alloca:
>
>   ``` %local = alloca %struct.Foo %tmpcast = bitcast %local call dbg.declare(%tmpcast, "local") <some instructions follow> %castOfTmpcast = bitcast %tmpcast ```
>
>   InstCombine recognizes that it can remove 'tmpcast' and simplify 'castOfTmpcast' if it promotes the type of the alloca. Since 'tmpcast' has only one use, we try to save its debug users, anticipating that 'tmpcast' will be deleted. The DomPoint / DT machinery gives us a way to prevent this use-before-def for the unlinked 'simplifiedCast':
>
>   ``` %local = alloca i64 call dbg.declare(%simplifiedCast, "local") <some instructions follow> %simplifiedCast = bitcast %local ```
>
>   We either salvage the debug user or delete it. The unit test has examples of both situations.
>
> > I think that we still have the problem with only checking dominance of the moved/replaced value though.
> >  As was discussed earlier somewhere (a patch in post RA MachineSink) we also might have the problem that there are other dbg.value intrinsics, referring to the same variable (or an overlapping fragment). And the question is if it is correct to reorder such dbg.value intrinsics, or what to do. Matt wrote a TR about it here https://bugs.llvm.org/show_bug.cgi?id=37897
>
> Doesn't this patch sidestep issues with reordering variable updates, because the updates are now all done in-place?


I thought that

  DII->moveAfter(&DomPoint);

still could move the DII without really taking other dbg.value intrinsics, potentially describing the same variable, into account.
But I see now that the moveAfter only happens when

  DomPointAfterFrom && DII->getNextNonDebugInstruction() == &DomPoint

which means that we do not move the DII arbitrarely (it's just moving to the other side of the DomPoint). So it should be ok.


https://reviews.llvm.org/D48676