-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[NFCI][MC][DecoderEmitter] Fix BitWidth for fixed length inst encodings #154934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
713aced
to
c7f7334
Compare
It sesms like ARM is easy to fix. Just don't underive Encoding16 from Encoding and provide its own Inst/SoftFail:
Assuming that the downstream targets are just as easy to fix, can we require Inst and Size to always match? |
Note that ignoring this MaxFilterWidth introduced in #154916 only affects a few lines in the generated file -- without it, some ExtractFields extract more bits than necessary. This should be harmless, I think. |
Change `InstructionEncoding` to use `Size` field to derive the BitWidth for fixed length instruction as opposed to the number of bits in the `Inst` field. For some backends, `Inst` has more bits that `Size`, but `Size` is the true size of the instruction. Also add validation that `Inst` has atleast `Size * 8` bits and any bits in `Inst` beyond that are either 0 or unset.
c7f7334
to
7192a82
Compare
It may be something easy to fix upstream, but I'd rather make this a warning and give time to folks (upstream and downstream) to fix and then make it an error (if Size * 8 != Inst field size). WDYT? |
✅ With the latest revision this PR passed the C/C++ code formatter. |
7192a82
to
867ab21
Compare
867ab21
to
3c37ad6
Compare
Looks like we started to use a new clang-format version upstream, so my local clang-format now not doing all the things expected. |
@llvm/pr-subscribers-tablegen Author: Rahul Joshi (jurahul) ChangesChange Also add validation that Verified no change in generated *GenDisassembler.inc files before/after. Full diff: https://github.com/llvm/llvm-project/pull/154934.diff 2 Files Affected:
diff --git a/llvm/test/TableGen/FixedLenDecoderEmitter/InvalidEncoding.td b/llvm/test/TableGen/FixedLenDecoderEmitter/InvalidEncoding.td
new file mode 100644
index 0000000000000..074c7c53e85c4
--- /dev/null
+++ b/llvm/test/TableGen/FixedLenDecoderEmitter/InvalidEncoding.td
@@ -0,0 +1,53 @@
+// RUN: not llvm-tblgen -gen-disassembler -I %p/../../../include %s -DTEST1 2>&1 | FileCheck %s --check-prefix=CHECK-TEST1
+// RUN: not llvm-tblgen -gen-disassembler -I %p/../../../include %s -DTEST2 2>&1 | FileCheck %s --check-prefix=CHECK-TEST2
+// RUN: not llvm-tblgen -gen-disassembler -I %p/../../../include %s -DTEST3 2>&1 | FileCheck %s --check-prefix=CHECK-TEST3
+
+include "llvm/Target/Target.td"
+
+def archInstrInfo : InstrInfo { }
+
+def arch : Target {
+ let InstructionSet = archInstrInfo;
+}
+
+#ifdef TEST1
+// CHECK-TEST1: [[#@LINE+1]]:5: error: foo: Size is 16 bits, but Inst bits beyond that are not zero/unset
+def foo : Instruction {
+ let OutOperandList = (outs);
+ let InOperandList = (ins i32imm:$factor);
+ let Size = 2;
+ field bits<24> Inst;
+ field bits<24> SoftFail = 0;
+ bits<8> factor;
+ let Inst{15...8} = factor{7...0};
+ let Inst{20} = 1;
+}
+#endif
+
+#ifdef TEST2
+// CHECK-TEST2: [[#@LINE+1]]:5: error: foo: Size is 16 bits, but SoftFail bits beyond that are not zero/unset
+def foo : Instruction {
+ let OutOperandList = (outs);
+ let InOperandList = (ins i32imm:$factor);
+ let Size = 2;
+ field bits<24> Inst;
+ field bits<24> SoftFail = 0;
+ bits<8> factor;
+ let Inst{15...8} = factor{7...0};
+ let Inst{23...16} = 0;
+ let SoftFail{20} = 1;
+}
+#endif
+
+#ifdef TEST3
+// CHECK-TEST3: [[#@LINE+1]]:5: error: bar: Size is 16 bits, but Inst specifies only 8 bits
+def bar : Instruction {
+ let OutOperandList = (outs);
+ let InOperandList = (ins i32imm:$factor);
+ let Size = 2;
+ field bits<8> Inst;
+ field bits<8> SoftFail = 0;
+ bits<4> factor;
+ let Inst{4...1} = factor{3...0};
+}
+#endif
diff --git a/llvm/utils/TableGen/DecoderEmitter.cpp b/llvm/utils/TableGen/DecoderEmitter.cpp
index 0d394ed87a4b8..02eb1410813bf 100644
--- a/llvm/utils/TableGen/DecoderEmitter.cpp
+++ b/llvm/utils/TableGen/DecoderEmitter.cpp
@@ -212,7 +212,7 @@ class InstructionEncoding {
private:
void parseVarLenEncoding(const VarLenInst &VLI);
- void parseFixedLenEncoding(const BitsInit &Bits);
+ void parseFixedLenEncoding(const BitsInit &RecordInstBits);
void parseVarLenOperands(const VarLenInst &VLI);
void parseFixedLenOperands(const BitsInit &Bits);
@@ -1787,12 +1787,51 @@ void InstructionEncoding::parseVarLenEncoding(const VarLenInst &VLI) {
assert(I == VLI.size());
}
-void InstructionEncoding::parseFixedLenEncoding(const BitsInit &Bits) {
- InstBits = KnownBits(Bits.getNumBits());
- SoftFailBits = APInt(Bits.getNumBits(), 0);
+void InstructionEncoding::parseFixedLenEncoding(
+ const BitsInit &RecordInstBits) {
+ // For fixed length instructions, sometimes the `Inst` field specifies more
+ // bits than the actual size of the instruction, which is specified in `Size`.
+ // In such cases, we do some basic validation and drop the upper bits.
+ unsigned BitWidth = EncodingDef->getValueAsInt("Size") * 8;
+ unsigned InstNumBits = RecordInstBits.getNumBits();
+
+ // Returns true if all bits in `Bits` are zero or unset.
+ auto CheckAllZeroOrUnset = [&](ArrayRef<const Init *> Bits,
+ const RecordVal *Field) {
+ bool AllZeroOrUnset = llvm::all_of(Bits, [](const Init *Bit) {
+ if (const auto *BI = dyn_cast<BitInit>(Bit))
+ return !BI->getValue();
+ return isa<UnsetInit>(Bit);
+ });
+ if (AllZeroOrUnset)
+ return;
+ PrintNote([Field](raw_ostream &OS) { Field->print(OS); });
+ PrintFatalError(EncodingDef, Twine(Name) + ": Size is " + Twine(BitWidth) +
+ " bits, but " + Field->getName() +
+ " bits beyond that are not zero/unset");
+ };
+
+ if (InstNumBits < BitWidth)
+ PrintFatalError(EncodingDef, Twine(Name) + ": Size is " + Twine(BitWidth) +
+ " bits, but Inst specifies only " +
+ Twine(InstNumBits) + " bits");
+
+ if (InstNumBits > BitWidth) {
+ // Ensure that all the bits beyond 'Size' are 0 or unset (i.e., carry no
+ // actual encoding).
+ ArrayRef<const Init *> UpperBits =
+ RecordInstBits.getBits().drop_front(BitWidth);
+ const RecordVal *InstField = EncodingDef->getValue("Inst");
+ CheckAllZeroOrUnset(UpperBits, InstField);
+ }
+
+ ArrayRef<const Init *> ActiveInstBits =
+ RecordInstBits.getBits().take_front(BitWidth);
+ InstBits = KnownBits(BitWidth);
+ SoftFailBits = APInt(BitWidth, 0);
// Parse Inst field.
- for (auto [I, V] : enumerate(Bits.getBits())) {
+ for (auto [I, V] : enumerate(ActiveInstBits)) {
if (const auto *B = dyn_cast<BitInit>(V)) {
if (B->getValue())
InstBits.One.setBit(I);
@@ -1802,26 +1841,36 @@ void InstructionEncoding::parseFixedLenEncoding(const BitsInit &Bits) {
}
// Parse SoftFail field.
- if (const RecordVal *SoftFailField = EncodingDef->getValue("SoftFail")) {
- const auto *SFBits = dyn_cast<BitsInit>(SoftFailField->getValue());
- if (!SFBits || SFBits->getNumBits() != Bits.getNumBits()) {
- PrintNote(EncodingDef->getLoc(), "in record");
- PrintFatalError(SoftFailField,
- formatv("SoftFail field, if defined, must be "
- "of the same type as Inst, which is bits<{}>",
- Bits.getNumBits()));
- }
- for (auto [I, V] : enumerate(SFBits->getBits())) {
- if (const auto *B = dyn_cast<BitInit>(V); B && B->getValue()) {
- if (!InstBits.Zero[I] && !InstBits.One[I]) {
- PrintNote(EncodingDef->getLoc(), "in record");
- PrintError(SoftFailField,
- formatv("SoftFail{{{0}} = 1 requires Inst{{{0}} "
- "to be fully defined (0 or 1, not '?')",
- I));
- }
- SoftFailBits.setBit(I);
+ const RecordVal *SoftFailField = EncodingDef->getValue("SoftFail");
+ if (!SoftFailField)
+ return;
+
+ const auto *SFBits = dyn_cast<BitsInit>(SoftFailField->getValue());
+ if (!SFBits || SFBits->getNumBits() != InstNumBits) {
+ PrintNote(EncodingDef->getLoc(), "in record");
+ PrintFatalError(SoftFailField,
+ formatv("SoftFail field, if defined, must be "
+ "of the same type as Inst, which is bits<{}>",
+ InstNumBits));
+ }
+
+ if (InstNumBits > BitWidth) {
+ // Ensure that all upper bits of `SoftFail` are 0 or unset.
+ ArrayRef<const Init *> UpperBits = SFBits->getBits().drop_front(BitWidth);
+ CheckAllZeroOrUnset(UpperBits, SoftFailField);
+ }
+
+ ArrayRef<const Init *> ActiveSFBits = SFBits->getBits().take_front(BitWidth);
+ for (auto [I, V] : enumerate(ActiveSFBits)) {
+ if (const auto *B = dyn_cast<BitInit>(V); B && B->getValue()) {
+ if (!InstBits.Zero[I] && !InstBits.One[I]) {
+ PrintNote(EncodingDef->getLoc(), "in record");
+ PrintError(SoftFailField,
+ formatv("SoftFail{{{0}} = 1 requires Inst{{{0}} "
+ "to be fully defined (0 or 1, not '?')",
+ I));
}
+ SoftFailBits.setBit(I);
}
}
}
|
Sounds reasonable |
Thanks, I am thinking of keeping this as is, and then adding the warning as a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/143/builds/10265 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/20644 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/11061 Here is the relevant piece of the build log for the reference
|
Change
InstructionEncoding
to useSize
field to derive the BitWidth for fixed length instructions as opposed to the number of bits in theInst
field. For some backends,Inst
has more bits thanSize
, butSize
is the true size of the instruction.Also add validation that
Inst
has at leastSize * 8
bits and any bits inInst
beyond that are either 0 or unset.Verified no change in generated *GenDisassembler.inc files before/after.