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

Bjorn Pettersson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 11 14:41:04 PDT 2019


bjope added a reviewer: fhahn.
bjope added a comment.

In D68633#1705697 <https://reviews.llvm.org/D68633#1705697>, @yechunliang wrote:

> > So the root cause is rather that we treat an alloca being immediately preceeded by another alloca differrently from the case when it is preceeded by another kind of instruction. This happens also when having other instructions in between, and is not specific to dbg intrinsics (could be interesting to add a test case where you replace the dbg intrinsics by something else).
>
> Yes I think so, if the other instruction is not dbg instr which exist between two allocas, the InlineFunction with and without "-strip-debug" will make the same behavior, that should both erase second use_empty alloca. This patch is to fix the issue that debug instr impact InlineFunction generate different output.
>
> > So I think that the solution might be based on one of these ideas:
> > 
> > 1. Remove the check for use_empty in the outer loop.
> > 2. Add a check for !use_empty in the inner loop.
> > 3. Remove the inner loop (i.e only splice one alloca at a time).
>
> These good ideas should be talking about the design change of alloca inline or improvement of splice.  
>  Read from the code, I think about the alloca inline behavior like this: First detect one !use_empty alloca,  if next immediate instructions are allocas, even they are use_empty, they will all added one after one and move to caller together with first alloca. if other instruction (whatever dbg or others instrs) exist between allocas, the next alloca will check if is use_empty or not, if is use_empty then erase. Does this behavior correct, or could be improve? I don't know much about alloca inline. but seems the code run many years with this design.


I'm no expert on this part of the code either. But there is no reasonable logic in handling allocas differently depending on the existence of other allocas here afaict. It looks like it has been like that for over 10 years, but I doubt that it justifies adding yet another level of illogical handling here. This time handling dbg intrinsics differently depending on the existance of, possibly unrelated, alloca instructions. It would just make this an even bigger mess. Would be much better if we could fix what seems to have been a bug(?) for over a decade (which also would solve the debug invariance problem).

(Maybe you'll need another set of reviewers considering that the fix would impact the alloca inlining slightly, rather than just aiming at dbg intrinsics?)


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

https://reviews.llvm.org/D68633





More information about the llvm-commits mailing list