This is a good capture. The issue occurred when the beginning instruction of BB is CFI_INSTRUCTION, the case could simplify as below:

########################################
---
name:            test2
body:             |
  ...
  bb.2:
    CFI_INSTRUCTION def_cfa_offset 8
    MOV8mi $r13, 1, $noreg, 0, $noreg, 0
    RET 0
...
########################################

The SkipTopCFIAndReturn will move the iterators forward (implement ++I2) when meet CFIInstruction. If the begin() is exactly CFIInstruction, the I2 will be assigned as next "real" instruction and return by  ComputeCommonTailLength(), while handle ProfitableToMerge outside, the problem happen to deal with "if(I2 == MBB2->begin())".


With and without CFI_INSTRUCTION at the beginning will really make difference after BranchFolder optimization, is this normal behavior, or it is a bug? 

Is there any definition describe that both CFI and DEBUG should not impact CodeGen result?

>  It seems like CFI directives, at least sometimes, are generated even without -g. 

CFI (call frame information) is emitted whenever unwinding is a possibility.
That can be with -g, it can also be with -fexceptions (and other similar
codegen options).

So, if you're looking for bugs in debug-invariant code generation, I'd say
you should always add -fexceptions to avoid CFI-related differences.

> One thing that I'm not 100% sure about is if CFI-directives should be allowed to impact codegen or not.

I know they act as scheduling barriers, because they describe rather precisely
what is needed to unwind at any given instruction.  I haven't thought about
whether they should influence other optimizations, but conservatively they
should be required to exactly match if you're going to do tail merging or
other kinds of instruction-sequence merging.  Otherwise, unwinding through a
tail-merged sequence will sometimes do the wrong thing.

It seems like BlockFolder (BlockFolding.cpp) isn't caring that much about CFI instruction inside the tail, but it does make sure it isn't chopping of CFI instructions in the beginning of the block.

I'm not sure if that causes any problems (not aware of any trouble reports related to that), so I've tried to just fix the debug invariant problem I found here: https://reviews.llvm.org/D70091


One theory is that CFI instructions in the beginning of a basic block for example could be def_cfa_offset directives that kind of restores the state after a terminator in the previous block, and we can't just ignore those. 
On the other hand, CFI instructions that comes after an instruction usually describes the effect of that instruction (e.g. a push/pop that moves the SP). So if we fold away the tail of a BB and jump to some other BB with the same instruction sequence, then it does not really matter if we have compared the CFI instructions or not. The CFI instructions present in the BB we jump to should be correct for that position of the code.

Although, I'm not that familiar with how the CFI instructions are handled. So the theory above might be flaky. I've just tried to understand the logic in how CFI instructions has been dealt with in ComputeCommonTailLength (since  https://reviews.llvm.org/D42848 which is the patch that started to ignore CFI instructions when searching for a common tail).

Hi,

Bjorn wrote:
> It seems like BlockFolder (BlockFolding.cpp) isn't caring that much about 
> CFI instruction inside the tail, but it does make sure it isn't chopping of 
> CFI instructions in the beginning of the block.

I get the impression that this might be an artefact of its "advance then rewind" approach, rather than something deliberate. One of the comments in the large block you delete in D70091 illustrates a CFI_INSTRUCTION inside a tail that shouldn't be "chopped off".

Then again, I know pretty much nothing about CFI anyway.

Paul wrote:
> CFI (call frame information) is emitted whenever unwinding is a possibility.
> That can be with -g, it can also be with -fexceptions (and other similar
> codegen options).

This sounds unpleasant -- if I understand correctly, then if only '-g' was specified then we want to prioritise not changing codegen; but if -fexceptions was specified then codegen _should_ change to make exceptions operate correctly? That could then (potentially) mean two different ways of treating CFI_INSTRUCTION.

The -g invariance introduced by https://reviews.llvm.org/D66467 has now been fixed in rG898de302919b (https://reviews.llvm.org/D70091).

The fix restores the old behavior to say that a found common tail cover the whole basic block even if debug instructions are found before the tail.

As the earlier comments talk a lot about CFI directives I want to highlight that the fix does not change anything related to CFI directives. If such instructions are found before the common tail, then we still need to split the BB when doing a tail merge.

(In reply to Jeremy Morse from comment #4)
> Paul wrote:
> > CFI (call frame information) is emitted whenever unwinding is a possibility.
> > That can be with -g, it can also be with -fexceptions (and other similar
> > codegen options).
> 
> This sounds unpleasant -- if I understand correctly, then if only '-g' was
> specified then we want to prioritise not changing codegen; but if
> -fexceptions was specified then codegen _should_ change to make exceptions
> operate correctly? That could then (potentially) mean two different ways of
> treating CFI_INSTRUCTION.

No no no...  Both -fexceptions and -g want to be told how to correctly unwind,
and they use exactly the same information to do that.  If you say -fexceptions
without -g, the data is the same, but we name the section .eh_frame instead of
.debug_frame.  That is the *only* difference.

This is a correctness issue!  The unwind info for -g must be correct, in order
for the debugger to walk up the stack without lying to you.

My position is that -g should not introduce any codegen differences when
compared to -fexceptions.