[TableGen] Delete support for deprecated positional matching.

[jyknight]

Summary

After the work in a538d1f13a13 5351878ba196 372240dfe3d5, and
subsequently cleanup of all the in-tree targets, we can now delete the
support for positional operand matching!

[MaskRay]

Thanks!

[barannikov88]

LGTM, great job!

llvm/include/llvm/Target/Target.td

class InstrInfo {
   // pattern when possible.
   //
   // Normally, TableGen will issue an error if it can't infer the value of a
   // property that hasn't been set explicitly. When guessInstructionProperties
   // is set, it will guess a safe value instead.
   //
   // This option is a temporary migration help. It will go away.
   bit guessInstructionProperties = true;
-
-  // TableGen's instruction encoder generator has support for matching operands
-  // to bit-field variables both by name and by position. Support for matching
-  // by position is DEPRECATED, and WILL BE REMOVED. Positional matching is
-  // confusing to use, and makes it very easy to accidentally write buggy
-  // instruction definitions.
-  //
-  // In previous versions of LLVM, the ability to match operands by position was
-  // enabled unconditionally. It is now controllable by this option -- and
-  // disabled by default. The previous behavior can be restored by setting this
-  // option to true.
-  //
-  // This option is temporary, and will go away once all in-tree targets have
-  // migrated.
-  //
-  // TODO: clean up and remove these options.
-  bit useDeprecatedPositionallyEncodedOperands = false;
-
-  // If positional encoding rules are used for the encoder generator, they may
-  // also need to be used by the decoder generator -- if so, enable this
-  // variable.
-  //
-  // This option is a no-op unless useDeprecatedPositionallyEncodedOperands is
-  // true.
-  //
-  // This option is temporary, and will go away once all in-tree targets have
-  // migrated.
-  bit decodePositionallyEncodedOperands = false;
-
-  // When set, this indicates that there will be no overlap between those
-  // operands that are matched by ordering (positional operands) and those
-  // matched by name.
-  //
-  // This is a no-op unless useDeprecatedPositionallyEncodedOperands is true
-  // (though it does modify the "would've used positional operand XXX" error.)
-  //
-  // This option is temporary, and will go away once all in-tree targets have
-  // migrated.
-  bit noNamedPositionallyEncodedOperands = false;
 }
 
 // Standard Pseudo Instructions.
 // This list must match TargetOpcodes.def.
 // Only these instructions are allowed in the TargetOpcode namespace.
 // Ensure mayLoad and mayStore have a default value, so as not to break
 // targets that set guessInstructionProperties=0. Any local definition of
 // mayLoad/mayStore takes precedence over these default values.

llvm/test/TableGen/InsufficientPositionalOperands.td

-// RUN: not llvm-tblgen -gen-emitter -I %p/../../include %s 2>&1 | FileCheck %s --implicit-check-not=error:
-
-// Check that TableGen doesn't crash on insufficient positional
-// instruction operands.
-
-// TODO: remove this test when removing useDeprecatedPositionallyEncodedOperands
-
-include "llvm/Target/Target.td"
-
-def ArchInstrInfo : InstrInfo {
-  let useDeprecatedPositionallyEncodedOperands = 1;
-}
-
-def Arch : Target {
-  let InstructionSet = ArchInstrInfo;
-}
-
-def Reg : Register<"reg">;
-
-def Regs : RegisterClass<"foo", [i32], 0, (add Reg)>;
-
-// CHECK: error: Too few operands in record foo (no match for variable rs)
-// CHECK: note: Dumping record for previous error:
-def foo : Instruction {
-  bits<3> rd;
-  bits<3> rs;
-
-  bits<8> Inst;
-  let Inst{1-0} = 0;
-  let Inst{4-2} = rd;
-  let Inst{7-5} = rs;
-
-  let OutOperandList = (outs Regs:$xd);
-  let InOperandList = (ins);
-}

llvm/test/TableGen/MissingOperandField.td

 def Arch : Target {
   let InstructionSet = ArchInstrInfo;
 }
 
 def Reg : Register<"reg">;
 
 def Regs : RegisterClass<"foo", [i32], 0, (add Reg)>;
 
-// CHECK: error: No operand named rd in record foo (would've used positional operand #0 ('xd') sub-op #0 with useDeprecatedPositionallyEncodedOperands=true)
-// CHECK: error: No operand named rs in record foo (would've given 'too few operands' error with useDeprecatedPositionallyEncodedOperands=true)
+// CHECK: error: No operand named rd in record foo
+// CHECK: error: No operand named rs in record foo
 // CHECK: note: Dumping record for previous error:
 def foo : Instruction {
   bits<3> rd;
   bits<3> rs;
 
   bits<8> Inst;
   let Inst{1-0} = 0;
   let Inst{4-2} = rd;
   let Inst{7-5} = rs;
 
   let OutOperandList = (outs Regs:$xd);
   let InOperandList = (ins);
 }

llvm/utils/TableGen/CodeEmitterGen.cpp

public:
   void run(raw_ostream &o);
 
 private:
   int getVariableBit(const std::string &VarName, BitsInit *BI, int bit);
   std::string getInstructionCase(Record *R, CodeGenTarget &Target);
   std::string getInstructionCaseForEncoding(Record *R, Record *EncodingDef,
                                             CodeGenTarget &Target);
   bool addCodeToMergeInOperand(Record *R, BitsInit *BI,
-                               const std::string &VarName, unsigned &NumberedOp,
-                               std::set<unsigned> &NamedOpIndices,
+                               const std::string &VarName,
                                std::string &Case, CodeGenTarget &Target);
 
   void emitInstructionBaseValues(
       raw_ostream &o, ArrayRef<const CodeGenInstruction *> NumberedInstructions,
       CodeGenTarget &Target, int HwMode = -1);
   unsigned BitWidth;
   bool UseAPInt;
 };
int CodeEmitterGen::getVariableBit(const std::string &VarName,
   }
 
   return -1;
 }
 
 // Returns true if it succeeds, false if an error.
 bool CodeEmitterGen::addCodeToMergeInOperand(Record *R, BitsInit *BI,
                                              const std::string &VarName,
-                                             unsigned &NumberedOp,
-                                             std::set<unsigned> &NamedOpIndices,
                                              std::string &Case,
                                              CodeGenTarget &Target) {
   CodeGenInstruction &CGI = Target.getInstruction(R);
 
   // Determine if VarName actually contributes to the Inst encoding.
   int bit = BI->getNumBits()-1;
 
   // Scan for a bit that this contributed to.
bool CodeEmitterGen::addCodeToMergeInOperand(Record *R, BitsInit *BI,
   unsigned OpIdx;
   std::pair<unsigned, unsigned> SubOp;
   if (CGI.Operands.hasSubOperandAlias(VarName, SubOp)) {
     OpIdx = CGI.Operands[SubOp.first].MIOperandNo + SubOp.second;
   } else if (CGI.Operands.hasOperandNamed(VarName, OpIdx)) {
     // Get the machine operand number for the indicated operand.
     OpIdx = CGI.Operands[OpIdx].MIOperandNo;
   } else {
-    // Fall back to positional lookup. By default, we now disable positional
-    // lookup (and print an error, below), but even so, we'll do the lookup to
-    // help print a helpful diagnostic message.
-    //
-    // TODO: When we remove useDeprecatedPositionallyEncodedOperands, delete all
-    // this code, just leaving a "no operand named X in record Y" error.
-
-    unsigned NumberOps = CGI.Operands.size();
-    /// If this operand is not supposed to be emitted by the
-    /// generated emitter, skip it.
-    while (NumberedOp < NumberOps &&
-           (CGI.Operands.isFlatOperandNotEmitted(NumberedOp) ||
-              (!NamedOpIndices.empty() && NamedOpIndices.count(
-                CGI.Operands.getSubOperandNumber(NumberedOp).first)))) {
-      ++NumberedOp;
-    }
-
-    if (NumberedOp >=
-        CGI.Operands.back().MIOperandNo + CGI.Operands.back().MINumOperands) {
-      if (!Target.getInstructionSet()->getValueAsBit(
-              "useDeprecatedPositionallyEncodedOperands")) {
-        PrintError(R, Twine("No operand named ") + VarName + " in record " +
-                          R->getName() +
-                          " (would've given 'too few operands' error with "
-                          "useDeprecatedPositionallyEncodedOperands=true)");
-      } else {
-        PrintError(R, "Too few operands in record " + R->getName() +
-                          " (no match for variable " + VarName + ")");
-      }
-      return false;
-    }
+    PrintError(R, Twine("No operand named ") + VarName + " in record " + R->getName());
 
-    OpIdx = NumberedOp++;
-
-    if (!Target.getInstructionSet()->getValueAsBit(
-            "useDeprecatedPositionallyEncodedOperands")) {
-      std::pair<unsigned, unsigned> SO =
-          CGI.Operands.getSubOperandNumber(OpIdx);
-      std::string OpName = CGI.Operands[SO.first].Name;
-      PrintError(R, Twine("No operand named ") + VarName + " in record " +
-                        R->getName() + " (would've used positional operand #" +
-                        Twine(SO.first) + " ('" + OpName + "') sub-op #" +
-                        Twine(SO.second) +
-                        " with useDeprecatedPositionallyEncodedOperands=true)");
-      return false;
-    }
-  }
-
   if (CGI.Operands.isFlatOperandNotEmitted(OpIdx)) {
     PrintError(R, "Operand " + VarName + " used but also marked as not emitted!");
     return false;
   }
 
   std::pair<unsigned, unsigned> SO = CGI.Operands.getSubOperandNumber(OpIdx);
   std::string &EncoderMethodName =
       CGI.Operands[SO.first].EncoderMethodNames[SO.second];
std::string CodeEmitterGen::getInstructionCase(Record *R,
   }
   return getInstructionCaseForEncoding(R, R, Target);
 }
 
 std::string CodeEmitterGen::getInstructionCaseForEncoding(Record *R, Record *EncodingDef,
                                                           CodeGenTarget &Target) {
   std::string Case;
   BitsInit *BI = EncodingDef->getValueAsBitsInit("Inst");
-  unsigned NumberedOp = 0;
-  std::set<unsigned> NamedOpIndices;
-
-  // Collect the set of operand indices that might correspond to named
-  // operand, and skip these when assigning operands based on position.
-  if (Target.getInstructionSet()->
-       getValueAsBit("noNamedPositionallyEncodedOperands")) {
-    CodeGenInstruction &CGI = Target.getInstruction(R);
-    for (const RecordVal &RV : R->getValues()) {
-      unsigned OpIdx;
-      if (!CGI.Operands.hasOperandNamed(RV.getName(), OpIdx))
-        continue;
-
-      NamedOpIndices.insert(OpIdx);
-    }
-  }
 
   // Loop over all of the fields in the instruction, determining which are the
   // operands to the instruction.
   bool Success = true;
   for (const RecordVal &RV : EncodingDef->getValues()) {
     // Ignore fixed fields in the record, we're looking for values like:
     //    bits<5> RST = { ?, ?, ?, ?, ? };
     if (RV.isNonconcreteOK() || RV.getValue()->isComplete())
       continue;
 
     Success &=
-        addCodeToMergeInOperand(R, BI, std::string(RV.getName()), NumberedOp,
-                                NamedOpIndices, Case, Target);
+        addCodeToMergeInOperand(R, BI, std::string(RV.getName()),
+                                Case, Target);
   }
 
   if (!Success) {
     // Dump the record, so we can see what's going on...
     std::string E;
     raw_string_ostream S(E);
     S << "Dumping record for previous error:\n";
     S << *R;

llvm/utils/TableGen/DecoderEmitter.cpp

for (unsigned j = 0; j < Op.Constraints.size(); ++j) {
         TiedNames[TiedName] = MyName;
       }
     }
   }
 
   if (IsVarLenInst) {
     parseVarLenInstOperand(EncodingDef, InsnOperands, CGI);
   } else {
-    std::map<std::string, std::vector<OperandInfo>> NumberedInsnOperands;
-    std::set<std::string> NumberedInsnOperandsNoTie;
-    bool SupportPositionalDecoding =
-        Target.getInstructionSet()->getValueAsBit(
-            "useDeprecatedPositionallyEncodedOperands") &&
-        Target.getInstructionSet()->getValueAsBit(
-            "decodePositionallyEncodedOperands");
-    if (SupportPositionalDecoding) {
-      const std::vector<RecordVal> &Vals = Def.getValues();
-      unsigned NumberedOp = 0;
-
-      std::set<unsigned> NamedOpIndices;
-      if (Target.getInstructionSet()->getValueAsBit(
-              "noNamedPositionallyEncodedOperands"))
-        // Collect the set of operand indices that might correspond to named
-        // operand, and skip these when assigning operands based on position.
-        for (unsigned i = 0, e = Vals.size(); i != e; ++i) {
-          unsigned OpIdx;
-          if (!CGI.Operands.hasOperandNamed(Vals[i].getName(), OpIdx))
-            continue;
-
-          NamedOpIndices.insert(OpIdx);
-        }
-
-      for (unsigned i = 0, e = Vals.size(); i != e; ++i) {
-        // Ignore fixed fields in the record, we're looking for values like:
-        //    bits<5> RST = { ?, ?, ?, ?, ? };
-        if (Vals[i].isNonconcreteOK() || Vals[i].getValue()->isComplete())
-          continue;
-
-        // Determine if Vals[i] actually contributes to the Inst encoding.
-        unsigned bi = 0;
-        for (; bi < Bits.getNumBits(); ++bi) {
-          VarInit *Var = nullptr;
-          VarBitInit *BI = dyn_cast<VarBitInit>(Bits.getBit(bi));
-          if (BI)
-            Var = dyn_cast<VarInit>(BI->getBitVar());
-          else
-            Var = dyn_cast<VarInit>(Bits.getBit(bi));
-
-          if (Var && Var->getName() == Vals[i].getName())
-            break;
-        }
-
-        if (bi == Bits.getNumBits())
-          continue;
-
-        // Skip variables that correspond to explicitly-named operands.
-        unsigned OpIdx;
-        std::pair<unsigned, unsigned> SubOp;
-        if (CGI.Operands.hasSubOperandAlias(Vals[i].getName(), SubOp) ||
-            CGI.Operands.hasOperandNamed(Vals[i].getName(), OpIdx))
-          continue;
-
-        // Get the bit range for this operand:
-        unsigned bitStart = bi++, bitWidth = 1;
-        for (; bi < Bits.getNumBits(); ++bi) {
-          VarInit *Var = nullptr;
-          VarBitInit *BI = dyn_cast<VarBitInit>(Bits.getBit(bi));
-          if (BI)
-            Var = dyn_cast<VarInit>(BI->getBitVar());
-          else
-            Var = dyn_cast<VarInit>(Bits.getBit(bi));
-
-          if (!Var)
-            break;
-
-          if (Var->getName() != Vals[i].getName())
-            break;
-
-          ++bitWidth;
-        }
-
-        unsigned NumberOps = CGI.Operands.size();
-        while (NumberedOp < NumberOps &&
-               (CGI.Operands.isFlatOperandNotEmitted(NumberedOp) ||
-                (!NamedOpIndices.empty() &&
-                 NamedOpIndices.count(
-                     CGI.Operands.getSubOperandNumber(NumberedOp).first))))
-          ++NumberedOp;
-
-        OpIdx = NumberedOp++;
-
-        // OpIdx now holds the ordered operand number of Vals[i].
-        std::pair<unsigned, unsigned> SO =
-            CGI.Operands.getSubOperandNumber(OpIdx);
-        const std::string &Name = CGI.Operands[SO.first].Name;
-
-        LLVM_DEBUG(dbgs() << "Numbered operand mapping for " << Def.getName()
-                          << ": " << Name << "(" << SO.first << ", "
-                          << SO.second << ") => " << Vals[i].getName() << "\n");
-
-        std::string Decoder;
-        Record *TypeRecord = CGI.Operands[SO.first].Rec;
-
-        RecordVal *DecoderString = TypeRecord->getValue("DecoderMethod");
-        StringInit *String =
-            DecoderString ? dyn_cast<StringInit>(DecoderString->getValue())
-                          : nullptr;
-        if (String && String->getValue() != "")
-          Decoder = std::string(String->getValue());
-
-        if (Decoder == "" && CGI.Operands[SO.first].MIOperandInfo &&
-            CGI.Operands[SO.first].MIOperandInfo->getNumArgs()) {
-          Init *Arg = CGI.Operands[SO.first].MIOperandInfo->getArg(SO.second);
-          if (DefInit *DI = cast<DefInit>(Arg))
-            TypeRecord = DI->getDef();
-        }
-
-        bool isReg = false;
-        if (TypeRecord->isSubClassOf("RegisterOperand"))
-          TypeRecord = TypeRecord->getValueAsDef("RegClass");
-        if (TypeRecord->isSubClassOf("RegisterClass")) {
-          Decoder = "Decode" + TypeRecord->getName().str() + "RegisterClass";
-          isReg = true;
-        } else if (TypeRecord->isSubClassOf("PointerLikeRegClass")) {
-          Decoder = "DecodePointerLikeRegClass" +
-                    utostr(TypeRecord->getValueAsInt("RegClassKind"));
-          isReg = true;
-        }
-
-        DecoderString = TypeRecord->getValue("DecoderMethod");
-        String = DecoderString ? dyn_cast<StringInit>(DecoderString->getValue())
-                               : nullptr;
-        if (!isReg && String && String->getValue() != "")
-          Decoder = std::string(String->getValue());
-
-        RecordVal *HasCompleteDecoderVal =
-            TypeRecord->getValue("hasCompleteDecoder");
-        BitInit *HasCompleteDecoderBit =
-            HasCompleteDecoderVal
-                ? dyn_cast<BitInit>(HasCompleteDecoderVal->getValue())
-                : nullptr;
-        bool HasCompleteDecoder =
-            HasCompleteDecoderBit ? HasCompleteDecoderBit->getValue() : true;
-
-        OperandInfo OpInfo(Decoder, HasCompleteDecoder);
-        OpInfo.addField(bitStart, bitWidth, 0);
-
-        NumberedInsnOperands[Name].push_back(OpInfo);
-
-        // FIXME: For complex operands with custom decoders we can't handle tied
-        // sub-operands automatically. Skip those here and assume that this is
-        // fixed up elsewhere.
-        if (CGI.Operands[SO.first].MIOperandInfo &&
-            CGI.Operands[SO.first].MIOperandInfo->getNumArgs() > 1 && String &&
-            String->getValue() != "")
-          NumberedInsnOperandsNoTie.insert(Name);
-      }
-    }
-
     // For each operand, see if we can figure out where it is encoded.
     for (const auto &Op : InOutOperands) {
       Init *OpInit = Op.first;
       StringRef OpName = Op.second;
 
-      if (SupportPositionalDecoding) {
-        if (!NumberedInsnOperands[std::string(OpName)].empty()) {
-          llvm::append_range(InsnOperands,
-                             NumberedInsnOperands[std::string(OpName)]);
-          continue;
-        }
-        if (!NumberedInsnOperands[TiedNames[std::string(OpName)]].empty()) {
-          if (!NumberedInsnOperandsNoTie.count(
-                  TiedNames[std::string(OpName)])) {
-            // Figure out to which (sub)operand we're tied.
-            unsigned i =
-                CGI.Operands.getOperandNamed(TiedNames[std::string(OpName)]);
-            int tiedTo = CGI.Operands[i].getTiedRegister();
-            if (tiedTo == -1) {
-              i = CGI.Operands.getOperandNamed(OpName);
-              tiedTo = CGI.Operands[i].getTiedRegister();
-            }
-
-            if (tiedTo != -1) {
-              std::pair<unsigned, unsigned> SO =
-                  CGI.Operands.getSubOperandNumber(tiedTo);
-
-              InsnOperands.push_back(
-                  NumberedInsnOperands[TiedNames[std::string(OpName)]]
-                                      [SO.second]);
-            }
-          }
-          continue;
-        }
-      }
-
       // We're ready to find the instruction encoding locations for this operand.
 
       // First, find the operand type ("OpInit"), and sub-op names
       // ("SubArgDag") if present.
       DagInit *SubArgDag = dyn_cast<DagInit>(OpInit);
       if (SubArgDag)
         OpInit = SubArgDag->getOperator();
       Record *OpTypeRec = cast<DefInit>(OpInit)->getDef();