[PATCH] D35844: Correct dwarf unwind information in function epilogue

Tue Oct 10 10:13:52 PDT 2017

violetav added inline comments.

================
Comment at: include/llvm/CodeGen/MachineInstr.h:795

+  bool isDirective() const { return isDebugValue() || isCFIInstruction(); }
   bool isPHI() const {
----------------
iteratee wrote:
> violetav wrote:
> > iteratee wrote:
> > > MatzeB wrote:
> > > > violetav wrote:
> > > > > MatzeB wrote:
> > > > > > This name is unintuitive to me. And it feels like this isn't really a category of machine instructions but rather just a combination that `BranchFolding` is able to deal with after this patch.
> > > > > > 
> > > > > > Maybe it would be better to move this into BranchFolding.cpp then in the form of
> > > > > > `static bool isDirective(const MachineInstr &MI) { return MI.isDebugValue() || MI.isCFIInstruction(); }`.
> > > > > This is used in BranchFolding and TailDuplication. It was suggested previously to be done this way https://reviews.llvm.org/D18046?id=101198#inline-294697
> > > > @iteratee Have you seen isMetaInstruction() and isTransient()? This feels like yet another variation of machine instructions that don't produce code. However I don't know why this specific combination is interesting and the generic name and no comment doesn't help understanding that either.
> > > I hadn't.
> > > Violeta, are there any reasons those 2 categories won't work?
> > Four regression tests fail when isDirective() is replaced with isMetaInstruction():
> > avx512-mask-op.ll
> > conditional-tailcall.ll
> > extra-toc-reg-deps.ll
> > tail-dup-merge-loop-headers.ll
> > 
> > Different code gets generated because of some tail merging and tail duplicating that don't happen with clean version of the code. At first glance, it seems that presence of IMPLICIT_DEF affects BranchFolding, and presence of KILL affects TailDuplicator. I haven't looked at this further than comparing .s files.
> violetav:
> 
> For Tail duplication I'm certain that it's fine. It's just counting instructions, and so it should skip all the meta instructions.
> 
> If you post the diff for BranchFolding, I'll look at it. I'm guessing it's fine too, but I'm less certain.
> 
> It would be easier if you did that first in a separate patch.
> 
Hi,

Other than different code being generated in some tests, there are at least two problems when isMetaInstruction() is used instead of isDirective() in BranchFolding.
First of all, even without this patch (only when places where there's isDirective() in it are replaced with isMetaInstruction()) there is a test that fails with 'Bad machine code' errors. That's test/CodeGen/PowerPC/extra-toc-reg-deps.ll and it reports:

Bad machine code: MBB has more than one landing pad successor ***
function:    _ZN4Foam13checkTopologyERKNS_8polyMeshEbb
basic block: BB#49 lpad175 (0x16665d0)

Bad machine code: MBB exits via unconditional branch but doesn't have exactly one CFG successor! ***
function:    _ZN4Foam13checkTopologyERKNS_8polyMeshEbb
basic block: BB#49 lpad175 (0x16665d0)

Bad machine code: MBB has more than one landing pad successor ***
function:    _ZN4Foam13checkTopologyERKNS_8polyMeshEbb
basic block: BB#53 lpad230 (0x1666da0)

Bad machine code: MBB exits via unconditional branch but doesn't have exactly one CFG successor! ***
function:    _ZN4Foam13checkTopologyERKNS_8polyMeshEbb
basic block: BB#53 lpad230 (0x1666da0)

Bad machine code: MBB has more than one landing pad successor ***
function:    _ZN4Foam13checkTopologyERKNS_8polyMeshEbb
basic block: BB#63 _ZN4Foam4ListIiED2Ev.exit.i3073 (0x1668510)

Bad machine code: MBB exits via unconditional branch but doesn't have exactly one CFG successor! ***
function:    _ZN4Foam13checkTopologyERKNS_8polyMeshEbb
basic block: BB#63 _ZN4Foam4ListIiED2Ev.exit.i3073 (0x1668510)

Bad machine code: MBB has more than one landing pad successor ***
function:    _ZN4Foam13checkTopologyERKNS_8polyMeshEbb
basic block: BB#70 lpad905.loopexit.split-lp (0x1668e00)

Bad machine code: MBB exits via unconditional branch but doesn't have exactly one CFG successor! ***
function:    _ZN4Foam13checkTopologyERKNS_8polyMeshEbb
basic block: BB#70 lpad905.loopexit.split-lp (0x1668e00)
LLVM ERROR: Found 8 machine code errors.

The second problem is seen in test/CodeGen/X86/conditional-tailcall.ll, which fails with following error:

The incoming offset of a successor is inconsistent. ***
function:    pr31257
basic block: BB#12 if.else28 (0x903c28)
Successor BB#14 has incoming offset (32), while BB#12 has outgoing offset (48).

The outgoing offset of a predecessor is inconsistent. ***
function:    pr31257
basic block: BB#14 cleanup.thread (0x903e98)
Predecessor BB#12 has outgoing offset (48), while BB#14 has incoming offset (32).
LLVM ERROR: Found 2 in/out CFI information errors.

What happens here is that a case appears where an iterator that points to the start of the common tail, points to a CFI instruction, and then that CFI instruction gets cut off, with a jump to the common tail, and gets lost. This then results in setting wrong outgoing CFA offset for that block (because one CFI instruction is missing), and the CFI Info Verifier reports a mismatch between outgoing offset of that block and incoming offset of its successor.

Errors about incorrect cfa offset are also reported with recursive LLVM build.

Considering these failures, I am not sure that isMetaInstruction() is safe to use here. I am not familiar with how these types of instructions should be handled.

It makes sense that isDirective() method is not added to MachineInstr, as it is used only for this purpose. If isMetaInstruction() turns out to be unsuitable for use here, we could, as Matthias suggested, add something such as shouldSkipInstr(MI) to BranchFolding and TailDuplication.

Repository:
  rL LLVM

https://reviews.llvm.org/D35844