updated patch:
[PHIEliminate] splice dbg instruction when LowerPHINode
DBG_VALUEs between PHI and LABEL will block PHI lownering after LABEL, move all DBG_VALUEs after Label before PHI lowering.

ok, I'll update the patch later.

My proposed code is as below: moving debug instrs after LastPHIIt at the beginning of PHIElimination

We try to move DBG_VALUEs after PHI and LABELs while ScheduleDAGSDNodes on early stage, but there is the the case the DBG_VALUEs exist between PHI and LABELs after Later PASS. Looks like we need to move DBG_VALUEs after LABELs on the beginning of PHIElimination.

Checked with extracted.ll, on the block [19]: the dbg.value did not skip "llvm.lifetime" and "%21 add" move after Label by SkipPHIsAndLabels() while ScheduleDAGSDNodes.
And later PASS (after ScheduleDAGSDNodes) removed "call llvm.lifetime.start" and "%21 add", and then left DBG_VALUEs between PHI and LABELs, and make errors.

In D70597#1810125, @jmorse wrote:

LGTM, thanks for working through this. Do you still need patches to be committed for you?

update patch with:

1. rename dbg_values as DBG_VALUEs to keep consistent.
2. add MBB.empty check at the beginning of MachineVerifier::checkPHIDbg

In D70597#1807759, @jmorse wrote:

Running check-llvm, there are a bunch of crashes, for example llvm/test/CodeGen/X86/win-catchpad.ll so there's certainly something breaking there.

fix code style issue.

Added checkPHIDbg to check "no dbg_values between PHI and labels" in MachineVerifier.

Look like adding one more method for SkipPHIsAndLabels with const_iterator would be more easier. I thought will be more duplicate code to implement SkipPHIsAndLabels and SkipPHIsLabelsAndDebug in MachineVerifier.
As the "No debug in PHI and labels" assert already be added in PHIElimination.cpp, is that safe if we do not add the checking in MachineVerifier?

Just updated the code [Diff 233770] follow the comments by jmose and bjope.
I try to add below reference code to MachineVerifier::checkPHIOps, but meet a build error. In the function the type of MBB.begin() is const_iterator, but type of SkipPHIsAndLabels argument is (iterator I). How to convert const_iterator to iterator and pass to SkipPHIsAndLabels? Could I write the MachineVerifier as below code?

@bjope, thanks for comments, that's a good idea, better than remember "No debug in PHI and labels" in mind.

Base on the solution "Postpone insertion of CFI instructions (pull out - reinsert)", with the patch, the CFI Instruction has been pulled out, but the problem is need to find the "correct position" to re-insert the CFI Instruction.

description for updated revision:

After study the history of the PHIElimination code changes, finally, I found this issue had been fixed before, and later the fixing code had been removed when created the SkipPHIsAndLables as new helper.

In D70597#1769185, @jmorse wrote:

In D70597#1768869, @yechunliang wrote:

Another idea is to add one more helper like getFirstNonPHIsAndLabels(), replace getFirstNonPHI in ScheduleDAGSDNodes.cpp#L935.
Insert dbg_values after PHIs and Labels, so that there will be no dbg_values barrier between PHI and LABELS.

This sounds good to me -- there's an existing "SkipPHIsLabelsAndDebug" method in MachineBasicBlock, would that be sufficient?

Not creating this scenario in the first place sounds is a good plan.

Another idea is to add one more helper like getFirstNonPHIsAndLabels(), replace getFirstNonPHI in ScheduleDAGSDNodes.cpp#L935.
Insert dbg_values after PHIs and Labels, so that there will be no dbg_values barrier between PHI and LABELS.

I try to move DBG_VALUE after COPY, but have some doubts:

1. Is there the case that DBG_VALUE be exist after LABELs use %1 definition? if there is, we need to move all DBG_VALUEs, if not, we just need to move DBG_VALUE which located after PHIs.
2. After refactoring by using AfterPHIsIt instead of LastPHIIt, there is the case that the basic block only have two PHIs, AfterPHIslt will be null and pass to LowerPHINode(), not sure if passing argument(null) to a function is safety or not?

@probinson @bjope thanks for review.

Updated the code and here is the description:

update description in test case

ping

name: test3
body: |
bb.0:

TEST8rr killed renamable $al, renamable $al, implicit-def $eflags
JCC_1 %bb.1, 5, implicit killed $eflags

@bjope thanks for the information. Without this patch, I have seem some check issues already there, I suggest to fix the issue first before apply this patch, than verify this patch. I don't know whether the CHECK in test case is correct or not, would you please help to provide test case without debug, help me find the normal behavior that could pass llc check without error, so that we could compare the result and diagnose why debug or cfi instructions impact the branch-folder pass. As this PR already merged, It's better for us to file a new bug and fix with another commit.

Look at the debugging printf information, I found the begin() is the first instruction of the BasicBlock, but the end() is not the last instruction, it's something unknown or none flag.
If what I found is correct, and would say end() could not be debug instruction, and maybe we needn't consider about debug instruction impact for end() part.

Will it be better to use getFirstNonDebugInstr() instead of begin() on line 1475?

@apilipenko thanks for accepting, I have no submit permission, would you please also help me to submit this patch? thanks.

@bjope,@jmorse, @aprantl, @jdoerfert, @vsk thank you all for kind and patient help on code review and giving me many supports and guidelines, I'm glad this(my first) patch be accepted and merged on llvm-project. Really thank you.

@aprantl @jmorse @bjope thanks for review, this patch is ready to land, but I have no submit permission, would you please help to submit it, many thanks.

@bjope thanks for help, to be honest, I don't know the process or execution of commit, I thought I have no commit permission right now.

Current status is "accepted and ready to land" for a long time.
I'm new on LLVM, Is there anything I need to do to make this PR be merged?

update test case:

1. using function name @foo and @bar instead of @f and @g for more complete match

2.cleanup test case with "-strip-dead-prototypes -strip-dead-debug-info"

just added use_empty check in the inner loop, this will be the smallest code change for issue fixing.

@bjope, @vsk: Thanks very much for the comments, really help me.

fix typo of patch description

Rethink about this issue and the root cause should be caused by use_empty. Just resubmitted the patch with "[InlineFunction] Erase use_empty allocas when inline".

! In D68633#1699421, @bjope wrote:
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).
   
   Alternative 3 would be the simplest. If there really is a speedup on doing fewer splices, then alternative 2 still moves consequtive !use_empty allocas in batches. The idea with alternative 2 is to split batches on allocas that has no uses (as they are handled in the outer loop). Alternative 1 might work assuming that allocas with no uses are cleaned up somewhere else. But I think this alternative is the least interesting one.

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.

Update patch:

1. Use BasicBlock::iterator skipDebugIntrinsics instead of rolling directly. The recommend API BasicBlock:: instructionsWithoutDebug() looks not very suitable to handle llvm::Instruction in this code.
2. follow llvm style, condition: break first, then unconditionally execute the code

In 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?).

[inline] skip dbg info when scan allocas

just added updated test, could some one help to review again? thanks.

Added one more RUN command line with "-debugify-each" in existed call-guard.ll, needn't duplicate another same test case file.

thanks for review. add test case with --debugify-each based on previous call-guard.ll

[InstCombine] Fix call guard difference with dbg

update branch-folder-with-debug.mir follow the review comments

• update CHECK block lines before successors check
• Add "CHECK-NEXT" for successors after block
• Simplify address after successors block
• move RUN line to top and add more descriptions.

@djtodoro thanks for review, followed your comments and guide from https://llvm.org/docs/MIRLangRef.html#id14, I have simplified mir test code by removing all unnecessary code for FileCheck.
"llc -o - test.mir -mtriple=x86_64-- -run-pass=branch-folder | FileCheck test.mir"

Simplify MIR test case

add branch-folder-with-debug.mir test

Please allow me to add Hsiangkai and Petar as reviewer, as you had contributed related patches about debug and CFI instruction skipping. Thanks.

thanks for review and comments. While adding test code, found another error about "unnamed alloca" before branch-folder by llc, I'll investigate the issue before submit the test code.