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

[Kyle Butt via Phabricator via llvm-commits]

iteratee added inline comments.


================
Comment at: include/llvm/CodeGen/MachineInstr.h:795
 
+  bool isDirective() const { return isDebugValue() || isCFIInstruction(); }
   bool isPHI() const {
----------------
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.



Repository:
  rL LLVM

https://reviews.llvm.org/D35844