Skip to content

Commit

Permalink
[TableGen] Delete support for deprecated positional matching.
Browse files Browse the repository at this point in the history 
After the work in a538d1f 5351878 372240d, and
subsequently cleanup of all the in-tree targets, we can now delete the
support for positional operand matching!

This removes three options which could previously be set in a
Target's "InstrInfo" tablegen definition:
- useDeprecatedPositionallyEncodedOperands
- decodePositionallyEncodedOperands
- noNamedPositionallyEncodedOperands

(Also announced at https://discourse.llvm.org/t/tablegen-deleting-deprecated-positional-instruction-operand-matching-support/68524)

Differential Revision: https://reviews.llvm.org/D144210
  • Loading branch information
@jyknight
jyknight committed Mar 7, 2023
1 parent 3ea4c50 commit b87dc35
Show file tree
Hide file tree
Showing 5 changed files with 7 additions and 326 deletions.
39 changes: 0 additions & 39 deletions llvm/include/llvm/Target/Target.td
View file
Expand Up @@ -1058,45 +1058,6 @@ class InstrInfo {
//
// 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.
Expand Down
35 changes: 0 additions & 35 deletions llvm/test/TableGen/InsufficientPositionalOperands.td
View file

This file was deleted.

4 changes: 2 additions & 2 deletions llvm/test/TableGen/MissingOperandField.td
View file
Expand Up @@ -15,8 +15,8 @@ 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;
Expand Down
73 changes: 5 additions & 68 deletions llvm/utils/TableGen/CodeEmitterGen.cpp
View file
Expand Up @@ -51,8 +51,7 @@ class CodeEmitterGen {
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(
Expand Down Expand Up @@ -81,8 +80,6 @@ int CodeEmitterGen::getVariableBit(const std::string &VarName,
// 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);
Expand Down Expand Up @@ -114,52 +111,8 @@ bool CodeEmitterGen::addCodeToMergeInOperand(Record *R, BitsInit *BI,
// 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;
}

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;
}
PrintError(R, Twine("No operand named ") + VarName + " in record " + R->getName());
return false;
}

if (CGI.Operands.isFlatOperandNotEmitted(OpIdx)) {
Expand Down Expand Up @@ -320,22 +273,6 @@ std::string CodeEmitterGen::getInstructionCaseForEncoding(Record *R, Record *Enc
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.
Expand All @@ -347,8 +284,8 @@ std::string CodeEmitterGen::getInstructionCaseForEncoding(Record *R, Record *Enc
continue;

Success &=
addCodeToMergeInOperand(R, BI, std::string(RV.getName()), NumberedOp,
NamedOpIndices, Case, Target);
addCodeToMergeInOperand(R, BI, std::string(RV.getName()),
Case, Target);
}

if (!Success) {
Expand Down
182 changes: 0 additions & 182 deletions llvm/utils/TableGen/DecoderEmitter.cpp
View file
Expand Up @@ -2029,193 +2029,11 @@ populateInstruction(CodeGenTarget &Target, const Record &EncodingDef,
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
Expand Down

0 comments on commit b87dc35

Please sign in to comment.