Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline
Changeset View
Standalone View
include/llvm/CodeGen/MachineInstr.h
| Show First 20 Lines • Show All 783 Lines • ▼ Show 20 Lines | public: | ||||
| /// A DBG_VALUE is indirect iff the first operand is a register and | /// A DBG_VALUE is indirect iff the first operand is a register and | ||||
| /// the second operand is an immediate. | /// the second operand is an immediate. | ||||
| bool isIndirectDebugValue() const { | bool isIndirectDebugValue() const { | ||||
| return isDebugValue() | return isDebugValue() | ||||
| && getOperand(0).isReg() | && getOperand(0).isReg() | ||||
| && getOperand(1).isImm(); | && getOperand(1).isImm(); | ||||
| } | } | ||||
| bool isDirective() const { return isDebugValue() || isCFIInstruction(); } | |||||
MatzeB: This name is unintuitive to me. And it feels like this isn't really a category of machine… | |||||
Not Done ReplyInline ActionsThis is used in BranchFolding and TailDuplication. It was suggested previously to be done this way https://reviews.llvm.org/D18046?id=101198#inline-294697 violetav: This is used in BranchFolding and TailDuplication. It was suggested previously to be done this… | |||||
Not Done ReplyInline Actions@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. MatzeB: @iteratee Have you seen isMetaInstruction() and isTransient()? This feels like yet another… | |||||
Not Done ReplyInline ActionsI hadn't. iteratee: I hadn't.
Violeta, are there any reasons those 2 categories won't work? | |||||
Not Done ReplyInline ActionsFour regression tests fail when isDirective() is replaced with isMetaInstruction(): 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: Four regression tests fail when isDirective() is replaced with isMetaInstruction():
avx512-mask… | |||||
Not Done ReplyInline Actionsvioletav: 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. iteratee: violetav:
For Tail duplication I'm certain that it's fine. It's just counting instructions… | |||||
Not Done ReplyInline ActionsHi, Other than different code being generated in some tests, there are at least two problems when isMetaInstruction() is used instead of isDirective() in BranchFolding. Bad machine code: MBB has more than one landing pad successor *** Bad machine code: MBB exits via unconditional branch but doesn't have exactly one CFG successor! *** Bad machine code: MBB has more than one landing pad successor *** Bad machine code: MBB exits via unconditional branch but doesn't have exactly one CFG successor! *** Bad machine code: MBB has more than one landing pad successor *** Bad machine code: MBB exits via unconditional branch but doesn't have exactly one CFG successor! *** Bad machine code: MBB has more than one landing pad successor *** Bad machine code: MBB exits via unconditional branch but doesn't have exactly one CFG successor! *** 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. *** The outgoing offset of a predecessor is inconsistent. *** 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. violetav: Hi,
Other than different code being generated in some tests, there are at least two problems… | |||||
| bool isPHI() const { return getOpcode() == TargetOpcode::PHI; } | bool isPHI() const { return getOpcode() == TargetOpcode::PHI; } | ||||
| bool isKill() const { return getOpcode() == TargetOpcode::KILL; } | bool isKill() const { return getOpcode() == TargetOpcode::KILL; } | ||||
| bool isImplicitDef() const { return getOpcode()==TargetOpcode::IMPLICIT_DEF; } | bool isImplicitDef() const { return getOpcode()==TargetOpcode::IMPLICIT_DEF; } | ||||
| bool isInlineAsm() const { return getOpcode() == TargetOpcode::INLINEASM; } | bool isInlineAsm() const { return getOpcode() == TargetOpcode::INLINEASM; } | ||||
| bool isMSInlineAsm() const { | bool isMSInlineAsm() const { | ||||
| return getOpcode() == TargetOpcode::INLINEASM && getInlineAsmDialect(); | return getOpcode() == TargetOpcode::INLINEASM && getInlineAsmDialect(); | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 547 Lines • Show Last 20 Lines | |||||
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(); }.