[Codegen][tablgen][NFC] Allow meta instruction to be target dependent

[skan]

Summary

An instruction is a meta-instruction if it doesn't produce any output
in the form of executable instructions. So in the concept, a
meta-instruction does not have to be target independent.

Before this patch, isMetaInstruction is implemented by checking the
opcode of the instruction, add we have no way to add target dependent
opcode to the list, which does not make sense.

After this patch, a bit isMeta is added for class Instruction in
tablegen, which is used to indicate whether it's a meta instruction.

[pengfei]

Is this an alternative of D92842?

llvm/include/llvm/MC/MCInstrDesc.h

268

Duplicated doesn't.

[skan]

In D121600#3379355, @pengfei wrote:

Is this an alternative of D92842?

No, these two patches have different purposes. This is to allow meta instruction to be target dependent, but D121600 is to unify the MEMBARRIER for different targets. But they can fix the same bug, which is a side effect.

[skan]

Fix typo in comments

[craig.topper]

If you’re fixing a bug shouldn’t there be a test?

[skan]

In D121600#3388065, @craig.topper wrote:

If you’re fixing a bug shouldn’t there be a test?

We have a unit test in MachineMetadata.cpp.

[craig.topper]

In D121600#3388072, @skan wrote:

In D121600#3388065, @craig.topper wrote:

If you’re fixing a bug shouldn’t there be a test?

We have a unit test in MachineMetadata.cpp.

Is that test doing anything more than checking that isMetaInstruction returns true for the test?

[skan]

In D121600#3388083, @craig.topper wrote:

In D121600#3388072, @skan wrote:

In D121600#3388065, @craig.topper wrote:

If you’re fixing a bug shouldn’t there be a test?

We have a unit test in MachineMetadata.cpp.

Is that test doing anything more than checking that isMetaInstruction returns true for the test?

It only checks isMetaInstruction returns true for Int_MemBarrier. There could be lots of bugs if isMetaInstruction returns false for an instruction that emits nothing executable. One of them is that the redundant dwarf debug location could be generated (llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp).
And unit test is the simplest way to check.

[pengfei]

Maybe break this into 2 patches. One for adding the target independent bit and one for bug fix?

[skan]

Split this patch: One for adding the target independent bit and one for bug fix.

[pengfei]

LGTM, but wait one or two days for other reviewers' opinions.

[skan]

In D121600#3388270, @pengfei wrote:

LGTM, but wait one or two days for other reviewers' opinions.

Thanks! A LIT test is provided at D121879.

[nigelp-xmos]

LGTM

[bjope]

llvm/include/llvm/CodeGen/MachineInstr.h

815

@skan (and reviewerts): Did you consider what the default QueryType should be here?

Downstream we've been using IgnoreBundle for isMetaInstruction (similarly as for isPseudo above). And now a number of debug info related test cases started to fail.

Before your patch isMetaInstruction() returned false when being queried on a BUNDLE instruction. So I think it would be more safe to use IgnoreBundle unless you want to change that behavior. And I don't think usage of isMetaInstruction in for example llvm::calculateDbgEntityHistory is supposed to skip a bundle just because one of multiple instructions in a BUNDLE is a meta instruction.

[skan]

llvm/include/llvm/CodeGen/MachineInstr.h

815

Thanks for the information! I prepared a patch here D122221.

llvm/include/llvm/CodeGen/MachineInstr.h

public:
   }
 
   /// Return true if this is a pseudo instruction that doesn't
   /// correspond to a real machine instruction.
   bool isPseudo(QueryType Type = IgnoreBundle) const {
     return hasProperty(MCID::Pseudo, Type);
   }
 
+
+  /// Return true if this instruction doesn't produce any output in the form of
+  /// executable instructions.
+  bool isMetaInstruction(QueryType Type = AnyInBundle) const {
+    return hasProperty(MCID::Meta, Type);
+  }
+
   bool isReturn(QueryType Type = AnyInBundle) const {
     return hasProperty(MCID::Return, Type);
   }
 
   /// Return true if this is an instruction that marks the end of an EH scope,
   /// i.e., a catchpad or a cleanuppad instruction.
   bool isEHScopeReturn(QueryType Type = AnyInBundle) const {
     return hasProperty(MCID::EHScopeReturn, Type);

   }
 
   /// Return true is the instruction is an identity copy.
   bool isIdentityCopy() const {
     return isCopy() && getOperand(0).getReg() == getOperand(1).getReg() &&
       getOperand(0).getSubReg() == getOperand(1).getSubReg();
   }
 
-  /// Return true if this instruction doesn't produce any output in the form of
-  /// executable instructions.
-  bool isMetaInstruction() const {
-    switch (getOpcode()) {
-    default:
-      return false;
-    case TargetOpcode::IMPLICIT_DEF:
-    case TargetOpcode::KILL:
-    case TargetOpcode::CFI_INSTRUCTION:
-    case TargetOpcode::EH_LABEL:
-    case TargetOpcode::GC_LABEL:
-    case TargetOpcode::DBG_VALUE:
-    case TargetOpcode::DBG_VALUE_LIST:
-    case TargetOpcode::DBG_INSTR_REF:
-    case TargetOpcode::DBG_PHI:
-    case TargetOpcode::DBG_LABEL:
-    case TargetOpcode::LIFETIME_START:
-    case TargetOpcode::LIFETIME_END:
-    case TargetOpcode::PSEUDO_PROBE:
-    case TargetOpcode::ARITH_FENCE:
-      return true;
-    }
-  }
-
   /// Return true if this is a transient instruction that is either very likely
   /// to be eliminated during register allocation (such as copy-like
   /// instructions), or if this instruction doesn't have an execution-time cost.
   bool isTransient() const {
     switch (getOpcode()) {
     default:
       return isMetaInstruction();
     // Copy-like instructions are usually eliminated during register allocation.

llvm/include/llvm/MC/MCInstrDesc.h

 /// MCInstrDesc class.  Clients should use the predicate methods on MCInstrDesc,
 /// not use these directly.  These all correspond to bitfields in the
 /// MCInstrDesc::Flags field.
 enum Flag {
   PreISelOpcode = 0,
   Variadic,
   HasOptionalDef,
   Pseudo,
+  Meta,
   Return,
   EHScopeReturn,
   Call,
   Barrier,
   Terminator,
   Branch,
   IndirectBranch,
   Compare,
public:
   /// Set if this instruction has an optional definition, e.g.
   /// ARM instructions which can set condition code if 's' bit is set.
   bool hasOptionalDef() const { return Flags & (1ULL << MCID::HasOptionalDef); }
 
   /// Return true if this is a pseudo instruction that doesn't
   /// correspond to a real machine instruction.
   bool isPseudo() const { return Flags & (1ULL << MCID::Pseudo); }
 
+  /// Return true if this is a meta instruction that doesn't
+  /// doesn't produce any output in the form of executable instructions
+  bool isMetaInstruction() const { return Flags & (1ULL << MCID::Meta); }
+
   /// Return true if the instruction is a return.
   bool isReturn() const { return Flags & (1ULL << MCID::Return); }
 
   /// Return true if the instruction is an add instruction.
   bool isAdd() const { return Flags & (1ULL << MCID::Add); }
 
   /// Return true if this instruction is a trap.
   bool isTrap() const { return Flags & (1ULL << MCID::Trap); }

llvm/include/llvm/Target/Target.td

class Instruction : InstructionEncoding {
   bit hasExtraSrcRegAllocReq = false; // Sources have special regalloc requirement?
   bit hasExtraDefRegAllocReq = false; // Defs have special regalloc requirement?
   bit isRegSequence = false;    // Is this instruction a kind of reg sequence?
                                 // If so, make sure to override
                                 // TargetInstrInfo::getRegSequenceLikeInputs.
   bit isPseudo     = false;     // Is this instruction a pseudo-instruction?
                                 // If so, won't have encoding information for
                                 // the [MC]CodeEmitter stuff.
+  bit isMeta = false;           // Is this instruction a meta-instruction?
+                                // If so, won't produce any output in the form of
+                                // executable instructions
   bit isExtractSubreg = false;  // Is this instruction a kind of extract subreg?
                                 // If so, make sure to override
                                 // TargetInstrInfo::getExtractSubregLikeInputs.
   bit isInsertSubreg = false;   // Is this instruction a kind of insert subreg?
                                 // If so, make sure to override
                                 // TargetInstrInfo::getInsertSubregLikeInputs.
   bit variadicOpsAreDefs = false; // Are variadic operands definitions?
 

 }
 def CFI_INSTRUCTION : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "";
   let hasCtrlDep = true;
   let hasSideEffects = false;
   let isNotDuplicable = true;
+  let isMeta = true;
 }
 def EH_LABEL : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "";
   let hasCtrlDep = true;
   let hasSideEffects = false;
   let isNotDuplicable = true;
+  let isMeta = true;
 }
 def GC_LABEL : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "";
   let hasCtrlDep = true;
   let hasSideEffects = false;
   let isNotDuplicable = true;
+  let isMeta = true;
 }
 def ANNOTATION_LABEL : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "";
   let hasCtrlDep = true;
   let hasSideEffects = false;
   let isNotDuplicable = true;
 }
 def KILL : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "";
   let hasSideEffects = false;
+  let isMeta = true;
 }
 def EXTRACT_SUBREG : StandardPseudoInstruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins unknown:$supersrc, i32imm:$subidx);
   let AsmString = "";
   let hasSideEffects = false;
 }
 def INSERT_SUBREG : StandardPseudoInstruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins unknown:$supersrc, unknown:$subsrc, i32imm:$subidx);
   let AsmString = "";
   let hasSideEffects = false;
   let Constraints = "$supersrc = $dst";
 }
 def IMPLICIT_DEF : StandardPseudoInstruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins);
   let AsmString = "";
   let hasSideEffects = false;
   let isReMaterializable = true;
   let isAsCheapAsAMove = true;
+  let isMeta = true;
 }
 def SUBREG_TO_REG : StandardPseudoInstruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins unknown:$implsrc, unknown:$subsrc, i32imm:$subidx);
   let AsmString = "";
   let hasSideEffects = false;
 }
 def COPY_TO_REGCLASS : StandardPseudoInstruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins unknown:$src, i32imm:$regclass);
   let AsmString = "";
   let hasSideEffects = false;
   let isAsCheapAsAMove = true;
 }
 def DBG_VALUE : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "DBG_VALUE";
   let hasSideEffects = false;
+  let isMeta = true;
 }
 def DBG_VALUE_LIST : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "DBG_VALUE_LIST";
   let hasSideEffects = 0;
+  let isMeta = true;
 }
 def DBG_INSTR_REF : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "DBG_INSTR_REF";
   let hasSideEffects = false;
+  let isMeta = true;
 }
 def DBG_PHI : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "DBG_PHI";
   let hasSideEffects = 0;
+  let isMeta = true;
 }
 def DBG_LABEL : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins unknown:$label);
   let AsmString = "DBG_LABEL";
   let hasSideEffects = false;
+  let isMeta = true;
 }
 def REG_SEQUENCE : StandardPseudoInstruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins unknown:$supersrc, variable_ops);
   let AsmString = "";
   let hasSideEffects = false;
   let isAsCheapAsAMove = true;
 }
def BUNDLE : StandardPseudoInstruction {
   let AsmString = "BUNDLE";
   let hasSideEffects = false;
 }
 def LIFETIME_START : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "LIFETIME_START";
   let hasSideEffects = false;
+  let isMeta = true;
 }
 def LIFETIME_END : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "LIFETIME_END";
   let hasSideEffects = false;
+  let isMeta = true;
 }
 def PSEUDO_PROBE : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i64imm:$guid, i64imm:$index, i8imm:$type, i32imm:$attr);
   let AsmString = "PSEUDO_PROBE";
   let hasSideEffects = 1;
+  let isMeta = true;
 }
 def ARITH_FENCE : StandardPseudoInstruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins unknown:$src);
   let AsmString = "";
   let hasSideEffects = false;
   let Constraints = "$src = $dst";
+  let isMeta = true;
 }
 
 def STACKMAP : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i64imm:$id, i32imm:$nbytes, variable_ops);
   let hasSideEffects = true;
   let isCall = true;
   let mayLoad = true;

llvm/lib/Target/X86/X86InstrCompiler.td

 // Memory barriers
 
 let isCodeGenOnly = 1, Defs = [EFLAGS] in
 def OR32mi8Locked  : Ii8<0x83, MRM1m, (outs), (ins i32mem:$dst, i32i8imm:$zero),
                          "or{l}\t{$zero, $dst|$dst, $zero}", []>,
                          Requires<[Not64BitMode]>, OpSize32, LOCK,
                          Sched<[WriteALURMW]>;
 
-let hasSideEffects = 1 in
+let hasSideEffects = 1, isMeta = 1 in
 def Int_MemBarrier : I<0, Pseudo, (outs), (ins),
                      "#MEMBARRIER",
                      [(X86MemBarrier)]>, Sched<[WriteLoad]>;
 
 // RegOpc corresponds to the mr version of the instruction
 // ImmOpc corresponds to the mi version of the instruction
 // ImmOpc8 corresponds to the mi8 version of the instruction
 // ImmMod corresponds to the instruction format of the mi and mi8 versions

llvm/lib/Target/XCore/XCoreInstrInfo.td

 let usesCustomInserter = 1 in {
   def SELECT_CC : PseudoInstXCore<(outs GRRegs:$dst),
                               (ins GRRegs:$cond, GRRegs:$T, GRRegs:$F),
                               "# SELECT_CC PSEUDO!",
                               [(set GRRegs:$dst,
                                  (select GRRegs:$cond, GRRegs:$T, GRRegs:$F))]>;
 }
 
-let hasSideEffects = 1 in
+let hasSideEffects = 1, isMeta = 1 in
 def Int_MemBarrier : PseudoInstXCore<(outs), (ins), "#MEMBARRIER",
                                      [(XCoreMemBarrier)]>;
 
 //===----------------------------------------------------------------------===//
 // Instructions
 //===----------------------------------------------------------------------===//
 
 // Three operand short

llvm/unittests/CodeGen/LexicalScopesTest.cpp

public:
   LexicalScopesTest() : Ctx(), Mod("beehives", Ctx) {
     memset(&BeanInst, 0, sizeof(BeanInst));
     BeanInst.Opcode = 1;
     BeanInst.Size = 1;
 
     memset(&DbgValueInst, 0, sizeof(DbgValueInst));
     DbgValueInst.Opcode = TargetOpcode::DBG_VALUE;
     DbgValueInst.Size = 1;
+    DbgValueInst.Flags = 1U << MCID::Meta;
 
     // Boilerplate that creates a MachineFunction and associated blocks.
     MF = createMachineFunction(Ctx, Mod);
     llvm::Function &F = const_cast<llvm::Function &>(MF->getFunction());
     auto BB1 = BasicBlock::Create(Ctx, "a", &F);
     auto BB2 = BasicBlock::Create(Ctx, "b", &F);
     auto BB3 = BasicBlock::Create(Ctx, "c", &F);
     auto BB4 = BasicBlock::Create(Ctx, "d", &F);

llvm/unittests/MIR/MachineMetadata.cpp

body:             |
   EH_LABEL 0
   GC_LABEL 0
   DBG_VALUE $rax, $noreg, !13, !DIExpression(), debug-location !14
   DBG_LABEL 0
   LIFETIME_START 0
   LIFETIME_END 0
   PSEUDO_PROBE 6699318081062747564, 1, 0, 0
   $xmm0 = ARITH_FENCE $xmm0
+  Int_MemBarrier
 ...
 )MIR";
 
   MachineModuleInfo MMI(TM.get());
   M = parseMIR(*TM, MIRString, "test0", MMI);
   ASSERT_TRUE(M);
 
   auto *MF = MMI.getMachineFunction(*M->getFunction("test0"));

llvm/utils/TableGen/CodeGenInstruction.h

public:
     bool isNotDuplicable : 1;
     bool hasSideEffects : 1;
     bool hasSideEffects_Unset : 1;
     bool isAsCheapAsAMove : 1;
     bool hasExtraSrcRegAllocReq : 1;
     bool hasExtraDefRegAllocReq : 1;
     bool isCodeGenOnly : 1;
     bool isPseudo : 1;
+    bool isMeta : 1;
     bool isRegSequence : 1;
     bool isExtractSubreg : 1;
     bool isInsertSubreg : 1;
     bool isConvergent : 1;
     bool hasNoSchedulingInfo : 1;
     bool FastISelShouldIgnore : 1;
     bool hasChain : 1;
     bool hasChain_Inferred : 1;

llvm/utils/TableGen/CodeGenInstruction.cpp

CodeGenInstruction::CodeGenInstruction(Record *R)
   hasSideEffects = R->getValueAsBitOrUnset("hasSideEffects", Unset);
   hasSideEffects_Unset = Unset;
 
   isAsCheapAsAMove = R->getValueAsBit("isAsCheapAsAMove");
   hasExtraSrcRegAllocReq = R->getValueAsBit("hasExtraSrcRegAllocReq");
   hasExtraDefRegAllocReq = R->getValueAsBit("hasExtraDefRegAllocReq");
   isCodeGenOnly = R->getValueAsBit("isCodeGenOnly");
   isPseudo = R->getValueAsBit("isPseudo");
+  isMeta = R->getValueAsBit("isMeta");
   ImplicitDefs = R->getValueAsListOfDefs("Defs");
   ImplicitUses = R->getValueAsListOfDefs("Uses");
 
   // This flag is only inferred from the pattern.
   hasChain = false;
   hasChain_Inferred = false;
 
   // Parse Constraints.

llvm/utils/TableGen/InstrInfoEmitter.cpp

OS << Num << ",\t" << MinOperands << ",\t"
      << Inst.TheDef->getValueAsInt("Size") << ",\t"
      << SchedModels.getSchedClassIdx(Inst) << ",\t0";
 
   CodeGenTarget &Target = CDP.getTargetInfo();
 
   // Emit all of the target independent flags...
   if (Inst.isPreISelOpcode)    OS << "|(1ULL<<MCID::PreISelOpcode)";
   if (Inst.isPseudo)           OS << "|(1ULL<<MCID::Pseudo)";
+  if (Inst.isMeta)             OS << "|(1ULL<<MCID::Meta)";
   if (Inst.isReturn)           OS << "|(1ULL<<MCID::Return)";
   if (Inst.isEHScopeReturn)    OS << "|(1ULL<<MCID::EHScopeReturn)";
   if (Inst.isBranch)           OS << "|(1ULL<<MCID::Branch)";
   if (Inst.isIndirectBranch)   OS << "|(1ULL<<MCID::IndirectBranch)";
   if (Inst.isCompare)          OS << "|(1ULL<<MCID::Compare)";
   if (Inst.isMoveImm)          OS << "|(1ULL<<MCID::MoveImm)";
   if (Inst.isMoveReg)          OS << "|(1ULL<<MCID::MoveReg)";
   if (Inst.isBitcast)          OS << "|(1ULL<<MCID::Bitcast)";