[TableGen] Add useDeprecatedPositionallyEncodedOperands option.

[jyknight]

Summary

The existing undefined-bitfield-to-operand matching behavior is very
hard to understand, due to the combination of positional and named
matching. This can make it difficult to track down a bug in a target's
instruction definitions.

Over the last decade, folks have tried to work-around this in various
ways, but it's time to finally ditch the positional matching. With
https://reviews.llvm.org/D131003, there are no longer cases that
_require_ positional matching, and it's time to start removing usage
and support for it.

Therefore: add a (default-false) option, and set it to true only in
those targets that require positional matching today. Subsequent
changes will start cleaning up additional in-tree targets.

NOTE TO OUT OF TREE TARGET MAINTAINERS:

If this change breaks your build, you may restore the previous
behavior simply by adding:

let useDeprecatedPositionallyEncodedOperands = 1;

to your target's InstrInfo tablegen definition. However, this is
temporary -- the option will be removed in the future.

If your target does not set 'decodePositionallyEncodedOperands', you
may thus start migrating to named operands. However, if you _do_
currently set that option, I recommend waiting until a subsequent
change lands, which adds decoder support for named sub-operands.

[MaskRay]

llvm/include/llvm/Target/Target.td

1052

Perhaps use a TODO as a minder that the support will be removed.

llvm/test/TableGen/InsufficientPositionalOperands.td

27

Add CHECK-NEXT: note: Dumping record for previous error: then use --implicit-check-not=error:

Q: by changing $xd, do we lose test coverage by not testing an undefined operand name?

llvm/utils/TableGen/CodeEmitterGen.cpp

53

addCodeToMergeInOperand now that the signature has changed

119

Does this need a TODO that this branch will be removed?

129

Drop the change to this code block

157

Since PrintError takes a Twine. You may remove E and S and just construct the long Twine as an argument

[jyknight]

Address review feedback. Also tweak error reporting to improve consistency for the "too few operands" case.

llvm/test/TableGen/InsufficientPositionalOperands.td

27

Done, and good point about losing test coverage. Switched this test to useDeprecatedPositionalOperands to preserve testing the positional error, and another test added for when positional matching is off.

[MaskRay]

Looks great!

llvm/utils/TableGen/DecoderEmitter.cpp

2134–2159

Does early return pattern apply?

llvm/include/llvm/Target/Target.td

class InstrInfo {
   // 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. While matching by
-  // name is preferred, this is currently not possible for complex operands,
-  // and some targets still reply on the positional encoding rules. When
-  // generating a decoder for such targets, the positional encoding rules must
-  // be used by the decoder generator as well.
-  //
-  // This option is temporary; it will go away once the TableGen decoder
-  // generator has better support for complex operands and targets have
-  // migrated away from using positionally encoded 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.
+  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 option is temporary; it will go away once the TableGen decoder
-  // generator has better support for complex operands and targets have
-  // migrated away from using positionally encoded operands.
+  // 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

llvm/lib/Target/AMDGPU/AMDGPU.td

def FeatureISAVersion11_0_3 : FeatureSet<
   !listconcat(FeatureISAVersion11_Common.Features,
     [])>;
 
 //===----------------------------------------------------------------------===//
 
 def AMDGPUInstrInfo : InstrInfo {
   let guessInstructionProperties = 1;
   let noNamedPositionallyEncodedOperands = 1;
+  let useDeprecatedPositionallyEncodedOperands = 1;
 }
 
 def AMDGPUAsmParser : AsmParser {
   // Some of the R600 registers have the same name, so this crashes.
   // For example T0_XYZW and T0_XY both have the asm name T0.
   let ShouldEmitMatchRegisterName = 0;
 }
 

llvm/lib/Target/AMDGPU/R600.td

 //===-- R600.td - R600 Tablegen files ----------------------*- tablegen -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 include "llvm/Target/Target.td"
 
 def R600InstrInfo : InstrInfo {
   let guessInstructionProperties = 1;
   let noNamedPositionallyEncodedOperands = 1;
+  let useDeprecatedPositionallyEncodedOperands = 1;
 }
 
 def R600 : Target {
   let InstructionSet = R600InstrInfo;
   let AllowRegisterRenaming = 1;
 }
 
 let Namespace = "R600" in {

llvm/lib/Target/AVR/AVR.td

 include "AVRRegisterInfo.td"
 
 //===---------------------------------------------------------------------===//
 // Instruction Descriptions
 //===---------------------------------------------------------------------===//
 
 include "AVRInstrInfo.td"
 
-def AVRInstrInfo : InstrInfo;
+def AVRInstrInfo : InstrInfo {
+  let useDeprecatedPositionallyEncodedOperands = true;
+}
 
 //===---------------------------------------------------------------------===//
 // Calling Conventions
 //===---------------------------------------------------------------------===//
 
 include "AVRCallingConv.td"
 
 //===---------------------------------------------------------------------===//

llvm/lib/Target/Lanai/Lanai.td

 // Register File, Calling Conv, Instruction Descriptions
 //===----------------------------------------------------------------------===//
 
 include "LanaiSchedule.td"
 include "LanaiRegisterInfo.td"
 include "LanaiCallingConv.td"
 include "LanaiInstrInfo.td"
 
-def LanaiInstrInfo : InstrInfo;
+def LanaiInstrInfo : InstrInfo {
+  let useDeprecatedPositionallyEncodedOperands = 1;
+}
 
 //===----------------------------------------------------------------------===//
 // Lanai processors supported.
 //===----------------------------------------------------------------------===//
 
 def : ProcessorModel<"generic", LanaiSchedModel, []>;
 def : ProcessorModel<"v11", LanaiSchedModel, []>;
 

llvm/lib/Target/Mips/Mips.td

 include "MipsCallingConv.td"
 include "MipsRegisterBanks.td"
 include "MipsCombine.td"
 
 // Avoid forward declaration issues.
 include "MipsScheduleP5600.td"
 include "MipsScheduleGeneric.td"
 
-def MipsInstrInfo : InstrInfo;
+def MipsInstrInfo : InstrInfo {
+ let useDeprecatedPositionallyEncodedOperands = 1;
+}
 
 //===----------------------------------------------------------------------===//
 // Mips processors supported.
 //===----------------------------------------------------------------------===//
 
 def ImplP5600 : SubtargetFeature<"p5600", "ProcImpl",
                                  "MipsSubtarget::CPU::P5600",
                                  "The P5600 Processor", [FeatureMips32r5]>;

llvm/lib/Target/PowerPC/PPC.td

 
 def PPCInstrInfo : InstrInfo {
   let isLittleEndianEncoding = 1;
 
   // FIXME: Unset this when no longer needed!
   let decodePositionallyEncodedOperands = 1;
 
   let noNamedPositionallyEncodedOperands = 1;
+  let useDeprecatedPositionallyEncodedOperands = 1;
 }
 
 def PPCAsmWriter : AsmWriter {
   string AsmWriterClassName  = "InstPrinter";
   int PassSubtarget = 1;
   int Variant = 0;
   bit isMCAsmWriter = 1;
 }

llvm/lib/Target/Sparc/Sparc.td

 // Register File, Calling Conv, Instruction Descriptions
 //===----------------------------------------------------------------------===//
 
 include "SparcRegisterInfo.td"
 include "SparcCallingConv.td"
 include "SparcSchedule.td"
 include "SparcInstrInfo.td"
 
-def SparcInstrInfo : InstrInfo;
+def SparcInstrInfo : InstrInfo {
+  let useDeprecatedPositionallyEncodedOperands = 1;
+}
 
 def SparcAsmParser : AsmParser {
   bit ShouldEmitMatchRegisterName = 0;
 }
 
 //===----------------------------------------------------------------------===//
 // SPARC processors supported.
 //===----------------------------------------------------------------------===//

llvm/lib/Target/VE/VE.td

 //===----------------------------------------------------------------------===//
 // Register File, Calling Conv, Instruction Descriptions
 //===----------------------------------------------------------------------===//
 
 include "VERegisterInfo.td"
 include "VECallingConv.td"
 include "VEInstrInfo.td"
 
-def VEInstrInfo : InstrInfo;
+def VEInstrInfo : InstrInfo {
+  let useDeprecatedPositionallyEncodedOperands = 1;
+}
 
 def VEAsmParser : AsmParser {
   // Use both VE register name matcher to accept "S0~S63" register names
   // and default register matcher to accept other registeres.
   let AllowDuplicateRegisterNames = 1;
   let ShouldEmitMatchRegisterAltName = 1;
 }
 

llvm/test/TableGen/InsufficientPositionalOperands.td

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;
 
-// CHECK: Too few operands in record foo (no match for variable rs)
-  let OutOperandList = (outs Regs:$xd);
+// CHECK: No operand named rs in record foo
+  let OutOperandList = (outs Regs:$rd);
   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);
-  void AddCodeToMergeInOperand(Record *R, BitsInit *BI,
-                               const std::string &VarName,
-                               unsigned &NumberedOp,
+  bool AddCodeToMergeInOperand(Record *R, BitsInit *BI,
+                               const std::string &VarName, unsigned &NumberedOp,
                                std::set<unsigned> &NamedOpIndices,
                                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,
   } else if (VarInit *VI = dyn_cast<VarInit>(BI->getBit(bit))) {
     if (VI->getName() == VarName)
       return 0;
   }
 
   return -1;
 }
 
-void CodeEmitterGen::
-AddCodeToMergeInOperand(Record *R, BitsInit *BI, const std::string &VarName,
+bool CodeEmitterGen::AddCodeToMergeInOperand(Record *R, BitsInit *BI,
+                                             const std::string &VarName,
                                              unsigned &NumberedOp,
                                              std::set<unsigned> &NamedOpIndices,
-                        std::string &Case, CodeGenTarget &Target) {
+                                             std::string &Case,
+                                             CodeGenTarget &Target) {
+  bool Success = true;
   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.
   for (; bit >= 0; ) {
     if (getVariableBit(VarName, BI, bit) != -1)
       break;
     
     --bit;
   }
   
   // If we found no bits, ignore this value, otherwise emit the call to get the
   // operand encoding.
-  if (bit < 0) return;
-  
+  if (bit < 0)
+    return Success;
   // If the operand matches by name, reference according to that
   // operand number. Non-matching operands are assumed to be in
   // order.
   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;
     assert(!CGI.Operands.isFlatOperandNotEmitted(OpIdx) &&
            "Explicitly used operand also marked as not emitted!");
   } 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.
+
     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(
+            (!NamedOpIndices.empty() &&
+             NamedOpIndices.count(
                  CGI.Operands.getSubOperandNumber(NumberedOp).first)))) {
       ++NumberedOp;
     }
 
     if (NumberedOp >=
         CGI.Operands.back().MIOperandNo + CGI.Operands.back().MINumOperands) {
       std::string E;
       raw_string_ostream S(E);
       S << "Too few operands in record " << R->getName()
         << " (no match for variable " << VarName << "):\n";
       S << *R;
       PrintFatalError(R, E);
     }
-
     OpIdx = NumberedOp++;
+
+    if (!Target.getInstructionSet()->getValueAsBit(
+            "useDeprecatedPositionallyEncodedOperands")) {
+      std::pair<unsigned, unsigned> SO =
+          CGI.Operands.getSubOperandNumber(OpIdx);
+      std::string OpName = CGI.Operands[SO.first].Name;
+      std::string E;
+      raw_string_ostream S(E);
+      S << "No operand named " << VarName << " in record " << R->getName()
+        << " ("
+        << "would've used positional operand #" << SO.first << " ('" << OpName
+        << "') sub-op #" << SO.second
+        << " with useDeprecatedPositionallyEncodedOperands=true)\n";
+      PrintError(R, E);
+      Success = false;
+    }
   }
 
   std::pair<unsigned, unsigned> SO = CGI.Operands.getSubOperandNumber(OpIdx);
   std::string &EncoderMethodName = CGI.Operands[SO.first].EncoderMethodName;
 
   if (UseAPInt)
     Case += "      op.clearAllBits();\n";
 
if (UseAPInt) {
           Case += "      Value |= (op & " + maskStr + ") >> " +
                   itostr(-opShift) + ";\n";
         } else {
           Case += "      Value |= (op & " + maskStr + ");\n";
         }
       }
     }
   }
+  return Success;
 }
 
 std::string CodeEmitterGen::getInstructionCase(Record *R,
                                                CodeGenTarget &Target) {
   std::string Case;
   if (const RecordVal *RV = R->getValue("EncodingInfos")) {
     if (auto *DI = dyn_cast_or_null<DefInit>(RV->getValue())) {
       const CodeGenHwModes &HWM = Target.getHwModes();
for (const RecordVal &RV : R->getValues()) {
         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);
   }
 
+  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;
+    PrintNote(E);
+  }
+
   StringRef PostEmitter = R->getValueAsString("PostEncoderMethod");
   if (!PostEmitter.empty()) {
     Case += "      Value = ";
     Case += PostEmitter;
     Case += "(MI, Value";
     Case += ", STI";
     Case += ");\n";
   }

llvm/utils/TableGen/DecoderEmitter.cpp

for (unsigned i = 0; i < CGI.Operands.size(); ++i) {
     }
   }
 
   if (IsVarLenInst) {
     parseVarLenInstOperand(EncodingDef, InsnOperands, CGI);
   } else {
     std::map<std::string, std::vector<OperandInfo>> NumberedInsnOperands;
     std::set<std::string> NumberedInsnOperandsNoTie;
-    if (Target.getInstructionSet()->getValueAsBit(
-            "decodePositionallyEncodedOperands")) {
+    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.
if (SupportPositionalDecoding) {
             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) {
+      if (SupportPositionalDecoding) {
         if (!NumberedInsnOperands[std::string(Op.second)].empty()) {
           llvm::append_range(InsnOperands,
                              NumberedInsnOperands[std::string(Op.second)]);
           continue;
         }
         if (!NumberedInsnOperands[TiedNames[std::string(Op.second)]].empty()) {
           if (!NumberedInsnOperandsNoTie.count(
                   TiedNames[std::string(Op.second)])) {
             // Figure out to which (sub)operand we're tied.
             unsigned i =
                 CGI.Operands.getOperandNamed(TiedNames[std::string(Op.second)]);
             int tiedTo = CGI.Operands[i].getTiedRegister();
             if (tiedTo == -1) {
               i = CGI.Operands.getOperandNamed(Op.second);
               tiedTo = CGI.Operands[i].getTiedRegister();
             }
 
             if (tiedTo != -1) {
               std::pair<unsigned, unsigned> SO =
                   CGI.Operands.getSubOperandNumber(tiedTo);
 
               InsnOperands.push_back(
                   NumberedInsnOperands[TiedNames[std::string(Op.second)]]
                                       [SO.second]);
             }
           }
           continue;
         }
+      }
 
       // At this point, we can locate the decoder field, but we need to know how
       // to interpret it.  As a first step, require the target to provide
       // callbacks for decoding register classes.
 
       Init *OpInit = Op.first;
       if (DagInit *Dag = dyn_cast<DagInit>(OpInit))
         OpInit = Dag->getOperator();