diff --git a/llvm/include/llvm/Target/Target.td b/llvm/include/llvm/Target/Target.td --- a/llvm/include/llvm/Target/Target.td +++ b/llvm/include/llvm/Target/Target.td @@ -1048,24 +1048,40 @@ 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. + // 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. // - // 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. + // 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; } diff --git a/llvm/lib/Target/AMDGPU/AMDGPU.td b/llvm/lib/Target/AMDGPU/AMDGPU.td --- a/llvm/lib/Target/AMDGPU/AMDGPU.td +++ b/llvm/lib/Target/AMDGPU/AMDGPU.td @@ -1307,6 +1307,7 @@ def AMDGPUInstrInfo : InstrInfo { let guessInstructionProperties = 1; let noNamedPositionallyEncodedOperands = 1; + let useDeprecatedPositionallyEncodedOperands = 1; } def AMDGPUAsmParser : AsmParser { diff --git a/llvm/lib/Target/AMDGPU/R600.td b/llvm/lib/Target/AMDGPU/R600.td --- a/llvm/lib/Target/AMDGPU/R600.td +++ b/llvm/lib/Target/AMDGPU/R600.td @@ -11,6 +11,7 @@ def R600InstrInfo : InstrInfo { let guessInstructionProperties = 1; let noNamedPositionallyEncodedOperands = 1; + let useDeprecatedPositionallyEncodedOperands = 1; } def R600 : Target { diff --git a/llvm/lib/Target/AVR/AVR.td b/llvm/lib/Target/AVR/AVR.td --- a/llvm/lib/Target/AVR/AVR.td +++ b/llvm/lib/Target/AVR/AVR.td @@ -32,7 +32,9 @@ include "AVRInstrInfo.td" -def AVRInstrInfo : InstrInfo; +def AVRInstrInfo : InstrInfo { + let useDeprecatedPositionallyEncodedOperands = true; +} //===---------------------------------------------------------------------===// // Calling Conventions diff --git a/llvm/lib/Target/Lanai/Lanai.td b/llvm/lib/Target/Lanai/Lanai.td --- a/llvm/lib/Target/Lanai/Lanai.td +++ b/llvm/lib/Target/Lanai/Lanai.td @@ -21,7 +21,9 @@ include "LanaiCallingConv.td" include "LanaiInstrInfo.td" -def LanaiInstrInfo : InstrInfo; +def LanaiInstrInfo : InstrInfo { + let useDeprecatedPositionallyEncodedOperands = 1; +} //===----------------------------------------------------------------------===// // Lanai processors supported. diff --git a/llvm/lib/Target/Mips/Mips.td b/llvm/lib/Target/Mips/Mips.td --- a/llvm/lib/Target/Mips/Mips.td +++ b/llvm/lib/Target/Mips/Mips.td @@ -223,7 +223,9 @@ include "MipsScheduleP5600.td" include "MipsScheduleGeneric.td" -def MipsInstrInfo : InstrInfo; +def MipsInstrInfo : InstrInfo { + let useDeprecatedPositionallyEncodedOperands = 1; +} //===----------------------------------------------------------------------===// // Mips processors supported. diff --git a/llvm/lib/Target/PowerPC/PPC.td b/llvm/lib/Target/PowerPC/PPC.td --- a/llvm/lib/Target/PowerPC/PPC.td +++ b/llvm/lib/Target/PowerPC/PPC.td @@ -664,6 +664,7 @@ let decodePositionallyEncodedOperands = 1; let noNamedPositionallyEncodedOperands = 1; + let useDeprecatedPositionallyEncodedOperands = 1; } def PPCAsmWriter : AsmWriter { diff --git a/llvm/lib/Target/Sparc/Sparc.td b/llvm/lib/Target/Sparc/Sparc.td --- a/llvm/lib/Target/Sparc/Sparc.td +++ b/llvm/lib/Target/Sparc/Sparc.td @@ -74,7 +74,9 @@ include "SparcSchedule.td" include "SparcInstrInfo.td" -def SparcInstrInfo : InstrInfo; +def SparcInstrInfo : InstrInfo { + let useDeprecatedPositionallyEncodedOperands = 1; +} def SparcAsmParser : AsmParser { bit ShouldEmitMatchRegisterName = 0; diff --git a/llvm/lib/Target/VE/VE.td b/llvm/lib/Target/VE/VE.td --- a/llvm/lib/Target/VE/VE.td +++ b/llvm/lib/Target/VE/VE.td @@ -30,7 +30,9 @@ 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 diff --git a/llvm/test/TableGen/InsufficientPositionalOperands.td b/llvm/test/TableGen/InsufficientPositionalOperands.td --- a/llvm/test/TableGen/InsufficientPositionalOperands.td +++ b/llvm/test/TableGen/InsufficientPositionalOperands.td @@ -24,7 +24,7 @@ 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); } diff --git a/llvm/utils/TableGen/CodeEmitterGen.cpp b/llvm/utils/TableGen/CodeEmitterGen.cpp --- a/llvm/utils/TableGen/CodeEmitterGen.cpp +++ b/llvm/utils/TableGen/CodeEmitterGen.cpp @@ -50,9 +50,8 @@ 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 &NamedOpIndices, std::string &Case, CodeGenTarget &Target); @@ -79,11 +78,13 @@ return -1; } -void CodeEmitterGen:: -AddCodeToMergeInOperand(Record *R, BitsInit *BI, const std::string &VarName, - unsigned &NumberedOp, - std::set &NamedOpIndices, - std::string &Case, CodeGenTarget &Target) { +bool CodeEmitterGen::AddCodeToMergeInOperand(Record *R, BitsInit *BI, + const std::string &VarName, + unsigned &NumberedOp, + std::set &NamedOpIndices, + std::string &Case, + CodeGenTarget &Target) { + bool Success = true; CodeGenInstruction &CGI = Target.getInstruction(R); // Determine if VarName actually contributes to the Inst encoding. @@ -99,8 +100,9 @@ // 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. @@ -114,13 +116,18 @@ 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( - CGI.Operands.getSubOperandNumber(NumberedOp).first)))) { + (!NamedOpIndices.empty() && + NamedOpIndices.count( + CGI.Operands.getSubOperandNumber(NumberedOp).first)))) { ++NumberedOp; } @@ -133,8 +140,23 @@ S << *R; PrintFatalError(R, E); } - OpIdx = NumberedOp++; + + if (!Target.getInstructionSet()->getValueAsBit( + "useDeprecatedPositionallyEncodedOperands")) { + std::pair 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 SO = CGI.Operands.getSubOperandNumber(OpIdx); @@ -266,6 +288,7 @@ } } } + return Success; } std::string CodeEmitterGen::getInstructionCase(Record *R, @@ -313,14 +336,25 @@ // 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; - AddCodeToMergeInOperand(R, BI, std::string(RV.getName()), NumberedOp, - NamedOpIndices, Case, Target); + 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"); diff --git a/llvm/utils/TableGen/DecoderEmitter.cpp b/llvm/utils/TableGen/DecoderEmitter.cpp --- a/llvm/utils/TableGen/DecoderEmitter.cpp +++ b/llvm/utils/TableGen/DecoderEmitter.cpp @@ -1982,8 +1982,12 @@ } else { std::map> NumberedInsnOperands; std::set NumberedInsnOperandsNoTie; - if (Target.getInstructionSet()->getValueAsBit( - "decodePositionallyEncodedOperands")) { + bool SupportPositionalDecoding = + Target.getInstructionSet()->getValueAsBit( + "useDeprecatedPositionallyEncodedOperands") && + Target.getInstructionSet()->getValueAsBit( + "decodePositionallyEncodedOperands"); + if (SupportPositionalDecoding) { const std::vector &Vals = Def.getValues(); unsigned NumberedOp = 0; @@ -2127,33 +2131,35 @@ // For each operand, see if we can figure out where it is encoded. for (const auto &Op : InOutOperands) { - 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 SO = - CGI.Operands.getSubOperandNumber(tiedTo); - - InsnOperands.push_back( - NumberedInsnOperands[TiedNames[std::string(Op.second)]] - [SO.second]); + 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 SO = + CGI.Operands.getSubOperandNumber(tiedTo); + + InsnOperands.push_back( + NumberedInsnOperands[TiedNames[std::string(Op.second)]] + [SO.second]); + } } + continue; } - continue; } // At this point, we can locate the decoder field, but we need to know how