Skip to content

Commit b103311

Browse files
topperctopperc
authored
[TableGen] Check destination instruction predicates in CompressInstEmitter. (#151061)
In addition to checking the predicate from the CompressPat, also check the destination instruction. This prevents creating bad instructions if CompressPat isn't a proper subset of the destination instruction. This prevents mistakes that we can't catch at compile time. We are able to verify RegisterClass hierarchy at compile time so don't have to check the destination register class. I've added comments for the operand names to make auditing easier.
1 parent dbcbdc4 commit b103311

File tree

2 files changed

+51
-26
lines changed

2 files changed

+51
-26
lines changed

‎llvm/test/TableGen/CompressInstEmitter/suboperands.td

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
115115
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(0).getReg()) &&
116116
// CHECK-NEXT: MI.getOperand(1).isReg() &&
117117
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
118-
// CHECK-NEXT: ArchValidateMCOperandForCompress(MI.getOperand(2), STI, 1)) {
118+
// CHECK-NEXT: ArchValidateMCOperandForCompress(MI.getOperand(2), STI, 1 /* simm6 */)) {
119119
// CHECK-NEXT: // small $dst, $addr
120120
// CHECK-NEXT: OutInst.setOpcode(Arch::SmallInst);
121121
// CHECK-NEXT: // Operand: dst
@@ -131,7 +131,7 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
131131
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(0).getReg()) &&
132132
// CHECK-NEXT: MI.getOperand(1).isReg() &&
133133
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
134-
// CHECK-NEXT: ArchValidateMCOperandForCompress(MI.getOperand(2), STI, 1)) {
134+
// CHECK-NEXT: ArchValidateMCOperandForCompress(MI.getOperand(2), STI, 1 /* simm6 */)) {
135135
// CHECK-NEXT: // small $dst, $src, $imm
136136
// CHECK-NEXT: OutInst.setOpcode(Arch::SmallInst2);
137137
// CHECK-NEXT: // Operand: dst
@@ -148,7 +148,7 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
148148
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(0).getReg()) &&
149149
// CHECK-NEXT: MI.getOperand(1).isReg() &&
150150
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
151-
// CHECK-NEXT: ArchValidateMCOperandForCompress(MI.getOperand(2), STI, 1)) {
151+
// CHECK-NEXT: ArchValidateMCOperandForCompress(MI.getOperand(2), STI, 1 /* simm6 */)) {
152152
// CHECK-NEXT: // small $dst, $addr
153153
// CHECK-NEXT: OutInst.setOpcode(Arch::SmallInst3);
154154
// CHECK-NEXT: // Operand: dst
@@ -170,7 +170,8 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
170170
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(0).getReg()) &&
171171
// CHECK-NEXT: MI.getOperand(1).isReg() &&
172172
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
173-
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 1)) {
173+
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 1 /* simm6 */) &&
174+
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 2 /* simm12 */))
174175
// CHECK-NEXT: // big $dst, $addr
175176
// CHECK-NEXT: OutInst.setOpcode(Arch::BigInst);
176177
// CHECK-NEXT: // Operand: dst
@@ -186,7 +187,8 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
186187
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(0).getReg()) &&
187188
// CHECK-NEXT: MI.getOperand(1).isReg() &&
188189
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
189-
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 1)) {
190+
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 1 /* simm6 */) &&
191+
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 2 /* simm12 */)) {
190192
// CHECK-NEXT: // big $dst, $addr
191193
// CHECK-NEXT: OutInst.setOpcode(Arch::BigInst2);
192194
// CHECK-NEXT: // Operand: dst
@@ -202,7 +204,8 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
202204
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(0).getReg()) &&
203205
// CHECK-NEXT: MI.getOperand(1).isReg() &&
204206
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
205-
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 1)) {
207+
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 1 /* simm6 */) &&
208+
// CHECK-NEXT: ArchValidateMCOperandForUncompress(MI.getOperand(2), STI, 2 /* simm12 */)) {
206209
// CHECK-NEXT: // big $dst, $src, $imm
207210
// CHECK-NEXT: OutInst.setOpcode(Arch::BigInst3);
208211
// CHECK-NEXT: // Operand: dst
@@ -226,7 +229,7 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
226229
// CHECK-NEXT: MI.getOperand(1).isReg() && MI.getOperand(1).getReg().isPhysical() &&
227230
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
228231
// CHECK-NEXT: MI.getOperand(2).isImm() &&
229-
// CHECK-NEXT: ArchValidateMachineOperand(MI.getOperand(2), &STI, 1)) {
232+
// CHECK-NEXT: ArchValidateMachineOperand(MI.getOperand(2), &STI, 1 /* simm6 */)) {
230233
// CHECK-NEXT: // small $dst, $addr
231234
// CHECK-NEXT: // Operand: dst
232235
// CHECK-NEXT: // Operand: addr
@@ -238,7 +241,7 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
238241
// CHECK-NEXT: MI.getOperand(1).isReg() && MI.getOperand(1).getReg().isPhysical() &&
239242
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
240243
// CHECK-NEXT: MI.getOperand(2).isImm() &&
241-
// CHECK-NEXT: ArchValidateMachineOperand(MI.getOperand(2), &STI, 1)) {
244+
// CHECK-NEXT: ArchValidateMachineOperand(MI.getOperand(2), &STI, 1 /* simm6 */)) {
242245
// CHECK-NEXT: // small $dst, $src, $imm
243246
// CHECK-NEXT: // Operand: dst
244247
// CHECK-NEXT: // Operand: src
@@ -251,7 +254,7 @@ def : CompressPat<(BigInst3 RegsC:$dst, RegsC:$src, simm6:$imm),
251254
// CHECK-NEXT: MI.getOperand(1).isReg() && MI.getOperand(1).getReg().isPhysical() &&
252255
// CHECK-NEXT: ArchMCRegisterClasses[Arch::RegsCRegClassID].contains(MI.getOperand(1).getReg()) &&
253256
// CHECK-NEXT: MI.getOperand(2).isImm() &&
254-
// CHECK-NEXT: ArchValidateMachineOperand(MI.getOperand(2), &STI, 1)) {
257+
// CHECK-NEXT: ArchValidateMachineOperand(MI.getOperand(2), &STI, 1 /* simm6 */)) {
255258
// CHECK-NEXT: // small $dst, $addr
256259
// CHECK-NEXT: // Operand: dst
257260
// CHECK-NEXT: // Operand: addr

‎llvm/utils/TableGen/CompressInstEmitter.cpp

Lines changed: 39 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -785,14 +785,14 @@ void CompressInstEmitter::emitCompressInstEmitter(raw_ostream &OS,
785785
switch (DestOperandMap[OpNo].Kind) {
786786
case OpData::Operand: {
787787
unsigned OpIdx = DestOperandMap[OpNo].OpInfo.Idx;
788-
DestRec = DestOperandMap[OpNo].OpInfo.DagRec;
788+
const Record *DagRec = DestOperandMap[OpNo].OpInfo.DagRec;
789789
// Check that the operand in the Source instruction fits
790790
// the type for the Dest instruction.
791-
if (DestRec->isSubClassOf("RegisterClass") ||
792-
DestRec->isSubClassOf("RegisterOperand")) {
793-
auto *ClassRec = DestRec->isSubClassOf("RegisterClass")
794-
? DestRec
795-
: DestRec->getValueAsDef("RegClass");
791+
if (DagRec->isSubClassOf("RegisterClass") ||
792+
DagRec->isSubClassOf("RegisterOperand")) {
793+
auto *ClassRec = DagRec->isSubClassOf("RegisterClass")
794+
? DagRec
795+
: DagRec->getValueAsDef("RegClass");
796796
// This is a register operand. Check the register class.
797797
// Don't check register class if this is a tied operand, it was done
798798
// for the operand it's tied to.
@@ -815,19 +815,40 @@ void CompressInstEmitter::emitCompressInstEmitter(raw_ostream &OS,
815815
// Handling immediate operands.
816816
if (CompressOrUncompress) {
817817
unsigned Entry = getPredicates(MCOpPredicateMap, MCOpPredicates,
818-
DestRec, "MCOperandPredicate");
819-
CondStream.indent(8) << ValidatorName << "("
820-
<< "MI.getOperand(" << OpIdx << "), STI, "
821-
<< Entry << ") &&\n";
818+
DagRec, "MCOperandPredicate");
819+
CondStream.indent(8)
820+
<< ValidatorName << "("
821+
<< "MI.getOperand(" << OpIdx << "), STI, " << Entry << " /* "
822+
<< DagRec->getName() << " */) &&\n";
823+
// Also check DestRec if different than DagRec.
824+
if (DagRec != DestRec) {
825+
Entry = getPredicates(MCOpPredicateMap, MCOpPredicates, DestRec,
826+
"MCOperandPredicate");
827+
CondStream.indent(8)
828+
<< ValidatorName << "("
829+
<< "MI.getOperand(" << OpIdx << "), STI, " << Entry
830+
<< " /* " << DestRec->getName() << " */) &&\n";
831+
}
822832
} else {
823833
unsigned Entry =
824-
getPredicates(ImmLeafPredicateMap, ImmLeafPredicates, DestRec,
834+
getPredicates(ImmLeafPredicateMap, ImmLeafPredicates, DagRec,
825835
"ImmediateCode");
826836
CondStream.indent(8)
827837
<< "MI.getOperand(" << OpIdx << ").isImm() &&\n";
828-
CondStream.indent(8) << TargetName << "ValidateMachineOperand("
829-
<< "MI.getOperand(" << OpIdx << "), &STI, "
830-
<< Entry << ") &&\n";
838+
CondStream.indent(8)
839+
<< TargetName << "ValidateMachineOperand("
840+
<< "MI.getOperand(" << OpIdx << "), &STI, " << Entry << " /* "
841+
<< DagRec->getName() << " */) &&\n";
842+
if (DagRec != DestRec) {
843+
Entry = getPredicates(ImmLeafPredicateMap, ImmLeafPredicates,
844+
DestRec, "ImmediateCode");
845+
CondStream.indent(8)
846+
<< "MI.getOperand(" << OpIdx << ").isImm() &&\n";
847+
CondStream.indent(8)
848+
<< TargetName << "ValidateMachineOperand("
849+
<< "MI.getOperand(" << OpIdx << "), &STI, " << Entry
850+
<< " /* " << DestRec->getName() << " */) &&\n";
851+
}
831852
}
832853
if (CompressOrUncompress)
833854
CodeStream.indent(6)
@@ -842,16 +863,17 @@ void CompressInstEmitter::emitCompressInstEmitter(raw_ostream &OS,
842863
CondStream.indent(8)
843864
<< ValidatorName << "("
844865
<< "MCOperand::createImm(" << DestOperandMap[OpNo].ImmVal
845-
<< "), STI, " << Entry << ") &&\n";
866+
<< "), STI, " << Entry << " /* " << DestRec->getName()
867+
<< " */) &&\n";
846868
} else {
847869
unsigned Entry =
848870
getPredicates(ImmLeafPredicateMap, ImmLeafPredicates, DestRec,
849871
"ImmediateCode");
850872
CondStream.indent(8)
851873
<< TargetName
852874
<< "ValidateMachineOperand(MachineOperand::CreateImm("
853-
<< DestOperandMap[OpNo].ImmVal << "), &STI, " << Entry
854-
<< ") &&\n";
875+
<< DestOperandMap[OpNo].ImmVal << "), &STI, " << Entry << " /* "
876+
<< DestRec->getName() << " */) &&\n";
855877
}
856878
if (CompressOrUncompress)
857879
CodeStream.indent(6) << "OutInst.addOperand(MCOperand::createImm("

0 commit comments

Comments
 (0)