[PATCH] D68633: fix debug info affects output when opt inline

[Chris Ye via Phabricator via llvm-commits]

yechunliang marked 5 inline comments as done.
yechunliang added a comment.

In D68633#1699421 <https://reviews.llvm.org/D68633#1699421>, @bjope wrote:

> The code is written in a way that it skips any instruction, but moves contigous blocks of allocas in one splice (not sure exactly why, is that really faster?).


I also could not understand why continue to scan allocas block after first none use_empty alloca instruction, here is the first commit has some reason:  https://github.com/llvm/llvm-project/commit/6f8865bf9

> Maybe the difference is that the check for AI->useEmpty() only is done for the first alloca in a sequence of alloca instructions? Or can't we just remove the loop at line 1847 (only moving one alloca at a time).

with this example test case, second alloca is use_empty, and will insert to caller together with first alloca (!use_empty). But if there is dbg instruction between first alloca and second alloca instruction. the continue scan will break, 
then with the debug instruction, the program will goto the front for() loop, and handle the second alloca as use_empty (because it has no user list like "xxx.sroa_cast = bitcast %rec1198* %volatileloadslot to i8*") and  eraseFromParent.
this is difference as no-dbg inline will not erase second alloca instruction.



================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1842-1843
 
+      // Debuginfo (@llvm.dbg.value) will make different result, skip while allocas scanning
+      while (isa<DbgInfoIntrinsic>(I)) ++I;
+
----------------
aprantl wrote:
> jmorse wrote:
> > Is there a possibility of an unrelated debug instruction being skipped here, and becoming part of the slice moved by lines 1847-1857? Moving dbg.values of arguments to the start of the caller may create a debug use-before-def situation, there could be other problem scenarios too.
> > 
> > Using a debug-instruction filtering iterator (like here [0]) might just do-the-right-thing, I don't know whether feeding one to splice would behave correctly though.
> > 
> > [0] https://github.com/llvm/llvm-project/blob/fdaa74217420729140f1786ea037ac445a724c8e/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L2592
> Don't we have an iterator that automatically skips debug intrinsics?
> Is there a possibility of an unrelated debug instruction being skipped here,
By default, the debug instruction between allocas will insert to caller together with block of allocas, I just keep the default behavier. Not sure if we need to remove the dbg instr when inline to the caller.  

> Using a debug-instruction filtering iterator (like here [0]) might just do-the-right-thing,
I could not found the way to use debug-instruction filtering like instructionsWithoutDebug(llvm::Instruction)  to handle BasicBlock::iterator. the format not match, so I used DbgInfoInstrinsic for simple usage.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1851
         ++I;
       }
 
----------------
jdoerfert wrote:
> Don't we need to have similar logic here? What happens if there are two allocas, then the dbg intrinsic, then another one?
thanks for the comments, this is a good case, I have updated code and testcase to handle this.


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

https://reviews.llvm.org/D68633