[PATCH] D68633: [utils] InlineFunction: fix for debug info affecting optimizations

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D68633#1709729 <https://reviews.llvm.org/D68633#1709729>, @vsk wrote:

> Hey @yechunliang, thanks for working on this.
>
> This part of the inliner looks like it has two issues: 1) it's not fully aware of debug instructions, and 2) it erroneously extends the lifetimes of inlined variables. The current iteration of your patch might address the first issue, but it's not DRY, and it doesn't solve the second.


Well, this part of the code is actually aware of dbg.declares (associated with the allocas), and moves those separately. This is done after the loop. And I guess that is (at least one reason to) why we save information about moves allocas in `IFI.StaticAllocas`.
Not sure if that is a bad thing to do or not, but that is probably a different problem. I think the initial goal here was to get rid of the debug invariance, i.e. that allocas could either be moved or removed depending on the presence of debug info.

My suggested solution to that is to handle all allocas the same (not treating two allocas in a row differently from two allocas separated by any other instruction). This makes this part of the code less sensitive to the order of instructions in the input. As a side effect (of such an improvement) the debug invariance problem is expected to go away.

> Here are a few suggestions.
> 
> - Please split out the logic for moving callee allocas into a helper function. This should simplify review.
> - Use the filtering iterator to skip debug instructions. I understand that you had some trouble with this. Here's a patch based on an older version of your diff: F10275020: inlinefunc.diff <https://reviews.llvm.org/F10275020>
> - Moving the dbg.declares along with the callee's allocas to the entry block erroneously extends the lifetimes of all those allocas to the whole function. This makes frame inspection in the debugger really confusing. To fix this, I think you should just not move the dbg.declares. The easiest way to do that is to stop batch-splicing, as @bjope pointed out.

I don't really see a reason for splitting this into a helper function. Nor starting to use a filtering iterator. My opinion is that the loop only should consider alloca instruction (just like it is today on trunk). But either the inner loop should be removed (only moving one alloca at a time), or if we still want to move sequences of alloca instructions, then the checks to determine if an alloca should be moved or not should be the same in the inner loop as in the outer loop. So the inner loop could simply look like this:

  while (isa<AllocaInst>(I) &&
         !cast<AllocaInst>(I)->use_empty() &&
         allocaWouldBeStaticInEntry(cast<AllocaInst>(I))) {
    IFI.StaticAllocas.push_back(cast<AllocaInst>(I));
    ++I;
  }

If we keep the inner loop the conditions for keeping/moving/removing an alloca is duplicated. So it would be much simpler just to stop doing batch moves.


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

https://reviews.llvm.org/D68633