[llvm] r336334 - [TableGen] Increase the number of supported decoder fix-ups.
Sander De Smalen via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 6 09:05:08 PDT 2018
Hi Ahmed,
Thanks for having another look at the patch. You're right that we can optimize this table quite a bit, indeed most of the 'skip-to' offsets seem to lead directly to OPC_Fail. Also, it seems that predicates are checked for every instruction individually, rather than for groups, so maybe these checks can be moved 'higher up' if all the encodings share the same predicate.
I like the idea of using ULEBs for the skip-to fields, but I'll need to give this a closer look to see what that would involve. The FixedLenDecoderEmitter generates the table recursively, reserving space for the skip-to offset that it fixes up later. I don't know if this would lead to lots of copying within the buffer to build up the decoder-table because the size of the offset itself isn't known yet.
Sander
On 05/07/2018, 21:38, "Ahmed Bougacha" <ahmed.bougacha at gmail.com> wrote:
On Thu, Jul 5, 2018 at 3:39 AM, Sander de Smalen via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> Author: s.desmalen
> Date: Thu Jul 5 03:39:15 2018
> New Revision: 336334
>
> URL: http://llvm.org/viewvc/llvm-project?rev=336334&view=rev
> Log:
> [TableGen] Increase the number of supported decoder fix-ups.
>
> The vast number of added instructions for SVE causes TableGen to fail with an assertion:
>
> Assertion `Delta < 65536U && "disassembler decoding table too large!"'
We also started hitting this downstream, but I'm wondering if we can
avoid the growth.
For instance, I was considering switching the offset to ULEB128, and
that's something we could probably do in a lot of generated
tables. It doesn't seem too complicated (we have APIs in libSupport,
thought they could perhaps be more user-friendly), or expensive (it's
a pretty simple scheme).
Alternatively, I looked at the table, and it seems that most of the
large deltas are just jumping to the final OPC_Fail. Why don't we
just special-case that with all 1s or something? Skimming over the
aarch64 tables, we probably don't even need more than 8 bits if we did
that.
WDYT?
-Ahmed
>
> This patch increases the number of supported decoder fix-ups.
>
> Reviewers: dmgreen, stoklund, petpav01
>
> Reviewed By: dmgreen
>
> Differential Revision: https://reviews.llvm.org/D48937
>
>
> Modified:
> llvm/trunk/test/TableGen/trydecode-emission.td
> llvm/trunk/test/TableGen/trydecode-emission2.td
> llvm/trunk/test/TableGen/trydecode-emission3.td
> llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp
>
> Modified: llvm/trunk/test/TableGen/trydecode-emission.td
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/TableGen/trydecode-emission.td?rev=336334&r1=336333&r2=336334&view=diff
> ==============================================================================
> --- llvm/trunk/test/TableGen/trydecode-emission.td (original)
> +++ llvm/trunk/test/TableGen/trydecode-emission.td Thu Jul 5 03:39:15 2018
> @@ -34,10 +34,10 @@ def InstB : TestInstruction {
> }
>
> // CHECK: /* 0 */ MCD::OPC_ExtractField, 4, 4, // Inst{7-4} ...
> -// CHECK-NEXT: /* 3 */ MCD::OPC_FilterValue, 0, 14, 0, // Skip to: 21
> -// CHECK-NEXT: /* 7 */ MCD::OPC_CheckField, 2, 2, 0, 5, 0, // Skip to: 18
> -// CHECK-NEXT: /* 13 */ MCD::OPC_TryDecode, {{[0-9]+}}, 0, 0, 0, // Opcode: InstB, skip to: 18
> -// CHECK-NEXT: /* 18 */ MCD::OPC_Decode, {{[0-9]+}}, 1, // Opcode: InstA
> -// CHECK-NEXT: /* 21 */ MCD::OPC_Fail,
> +// CHECK-NEXT: /* 3 */ MCD::OPC_FilterValue, 0, 16, 0, 0, // Skip to: 24
> +// CHECK-NEXT: /* 8 */ MCD::OPC_CheckField, 2, 2, 0, 6, 0, 0, // Skip to: 21
> +// CHECK-NEXT: /* 15 */ MCD::OPC_TryDecode, {{[0-9]+}}, 0, 0, 0, 0, // Opcode: InstB, skip to: 21
> +// CHECK-NEXT: /* 21 */ MCD::OPC_Decode, {{[0-9]+}}, 1, // Opcode: InstA
> +// CHECK-NEXT: /* 24 */ MCD::OPC_Fail,
>
> // CHECK: if (DecodeInstB(MI, insn, Address, Decoder) == MCDisassembler::Fail) { DecodeComplete = false; return MCDisassembler::Fail; }
>
> Modified: llvm/trunk/test/TableGen/trydecode-emission2.td
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/TableGen/trydecode-emission2.td?rev=336334&r1=336333&r2=336334&view=diff
> ==============================================================================
> --- llvm/trunk/test/TableGen/trydecode-emission2.td (original)
> +++ llvm/trunk/test/TableGen/trydecode-emission2.td Thu Jul 5 03:39:15 2018
> @@ -31,14 +31,14 @@ def InstB : TestInstruction {
> }
>
> // CHECK: /* 0 */ MCD::OPC_ExtractField, 2, 1, // Inst{2} ...
> -// CHECK-NEXT: /* 3 */ MCD::OPC_FilterValue, 0, 29, 0, // Skip to: 36
> -// CHECK-NEXT: /* 7 */ MCD::OPC_ExtractField, 5, 3, // Inst{7-5} ...
> -// CHECK-NEXT: /* 10 */ MCD::OPC_FilterValue, 0, 22, 0, // Skip to: 36
> -// CHECK-NEXT: /* 14 */ MCD::OPC_CheckField, 0, 2, 3, 5, 0, // Skip to: 25
> -// CHECK-NEXT: /* 20 */ MCD::OPC_TryDecode, {{[0-9]+}}, 0, 0, 0, // Opcode: InstB, skip to: 25
> -// CHECK-NEXT: /* 25 */ MCD::OPC_CheckField, 3, 2, 0, 5, 0, // Skip to: 36
> -// CHECK-NEXT: /* 31 */ MCD::OPC_TryDecode, {{[0-9]+}}, 1, 0, 0, // Opcode: InstA, skip to: 36
> -// CHECK-NEXT: /* 36 */ MCD::OPC_Fail,
> +// CHECK-NEXT: /* 3 */ MCD::OPC_FilterValue, 0, 34, 0, 0, // Skip to: 42
> +// CHECK-NEXT: /* 8 */ MCD::OPC_ExtractField, 5, 3, // Inst{7-5} ...
> +// CHECK-NEXT: /* 11 */ MCD::OPC_FilterValue, 0, 26, 0, 0, // Skip to: 42
> +// CHECK-NEXT: /* 16 */ MCD::OPC_CheckField, 0, 2, 3, 6, 0, 0, // Skip to: 29
> +// CHECK-NEXT: /* 23 */ MCD::OPC_TryDecode, {{[0-9]+}}, 0, 0, 0, 0, // Opcode: InstB, skip to: 29
> +// CHECK-NEXT: /* 29 */ MCD::OPC_CheckField, 3, 2, 0, 6, 0, 0, // Skip to: 42
> +// CHECK-NEXT: /* 36 */ MCD::OPC_TryDecode, {{[0-9]+}}, 1, 0, 0, 0, // Opcode: InstA, skip to: 42
> +// CHECK-NEXT: /* 42 */ MCD::OPC_Fail,
>
> // CHECK: if (DecodeInstB(MI, insn, Address, Decoder) == MCDisassembler::Fail) { DecodeComplete = false; return MCDisassembler::Fail; }
> // CHECK: if (DecodeInstA(MI, insn, Address, Decoder) == MCDisassembler::Fail) { DecodeComplete = false; return MCDisassembler::Fail; }
>
> Modified: llvm/trunk/test/TableGen/trydecode-emission3.td
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/TableGen/trydecode-emission3.td?rev=336334&r1=336333&r2=336334&view=diff
> ==============================================================================
> --- llvm/trunk/test/TableGen/trydecode-emission3.td (original)
> +++ llvm/trunk/test/TableGen/trydecode-emission3.td Thu Jul 5 03:39:15 2018
> @@ -35,10 +35,10 @@ def InstB : TestInstruction {
> }
>
> // CHECK: /* 0 */ MCD::OPC_ExtractField, 4, 4, // Inst{7-4} ...
> -// CHECK-NEXT: /* 3 */ MCD::OPC_FilterValue, 0, 14, 0, // Skip to: 21
> -// CHECK-NEXT: /* 7 */ MCD::OPC_CheckField, 2, 2, 0, 5, 0, // Skip to: 18
> -// CHECK-NEXT: /* 13 */ MCD::OPC_TryDecode, {{[0-9]+}}, 0, 0, 0, // Opcode: InstB, skip to: 18
> -// CHECK-NEXT: /* 18 */ MCD::OPC_Decode, {{[0-9]+}}, 1, // Opcode: InstA
> -// CHECK-NEXT: /* 21 */ MCD::OPC_Fail,
> +// CHECK-NEXT: /* 3 */ MCD::OPC_FilterValue, 0, 16, 0, 0, // Skip to: 24
> +// CHECK-NEXT: /* 8 */ MCD::OPC_CheckField, 2, 2, 0, 6, 0, 0, // Skip to: 21
> +// CHECK-NEXT: /* 15 */ MCD::OPC_TryDecode, {{[0-9]+}}, 0, 0, 0, 0, // Opcode: InstB, skip to: 21
> +// CHECK-NEXT: /* 21 */ MCD::OPC_Decode, {{[0-9]+}}, 1, // Opcode: InstA
> +// CHECK-NEXT: /* 24 */ MCD::OPC_Fail,
>
> // CHECK: if (DecodeInstBOp(MI, tmp, Address, Decoder) == MCDisassembler::Fail) { DecodeComplete = false; return MCDisassembler::Fail; }
>
> Modified: llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp?rev=336334&r1=336333&r2=336334&view=diff
> ==============================================================================
> --- llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp (original)
> +++ llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp Thu Jul 5 03:39:15 2018
> @@ -606,12 +606,13 @@ static void resolveTableFixups(DecoderTa
> // NumToSkip entry itself, so subtract two from the displacement here
> // to account for that.
> uint32_t FixupIdx = *I;
> - uint32_t Delta = DestIdx - FixupIdx - 2;
> - // Our NumToSkip entries are 16-bits. Make sure our table isn't too
> + uint32_t Delta = DestIdx - FixupIdx - 3;
> + // Our NumToSkip entries are 24-bits. Make sure our table isn't too
> // big.
> - assert(Delta < 65536U && "disassembler decoding table too large!");
> + assert(Delta < (1u << 24));
> Table[FixupIdx] = (uint8_t)Delta;
> Table[FixupIdx + 1] = (uint8_t)(Delta >> 8);
> + Table[FixupIdx + 2] = (uint8_t)(Delta >> 16);
> }
> }
>
> @@ -646,7 +647,7 @@ void Filter::emitTableEntry(DecoderTable
> } else {
> Table.push_back(MCD::OPC_FilterValue);
> // Encode and emit the value to filter against.
> - uint8_t Buffer[8];
> + uint8_t Buffer[16];
> unsigned Len = encodeULEB128(Filter.first, Buffer);
> Table.insert(Table.end(), Buffer, Buffer + Len);
> // Reserve space for the NumToSkip entry. We'll backpatch the value
> @@ -654,6 +655,7 @@ void Filter::emitTableEntry(DecoderTable
> PrevFilter = Table.size();
> Table.push_back(0);
> Table.push_back(0);
> + Table.push_back(0);
> }
>
> // We arrive at a category of instructions with the same segment value.
> @@ -666,10 +668,11 @@ void Filter::emitTableEntry(DecoderTable
> // of the filter itself to be able to skip forward when false. Subtract
> // two as to account for the width of the NumToSkip field itself.
> if (PrevFilter) {
> - uint32_t NumToSkip = Table.size() - PrevFilter - 2;
> - assert(NumToSkip < 65536U && "disassembler decoding table too large!");
> + uint32_t NumToSkip = Table.size() - PrevFilter - 3;
> + assert(NumToSkip < (1u << 24) && "disassembler decoding table too large!");
> Table[PrevFilter] = (uint8_t)NumToSkip;
> Table[PrevFilter + 1] = (uint8_t)(NumToSkip >> 8);
> + Table[PrevFilter + 2] = (uint8_t)(NumToSkip >> 16);
> }
> }
>
> @@ -745,13 +748,16 @@ void FixedLenDecoderEmitter::emitTable(f
> OS << (unsigned)*I++ << ", ";
> OS << (unsigned)*I++ << ", ";
>
> - // 16-bit numtoskip value.
> + // 24-bit numtoskip value.
> uint8_t Byte = *I++;
> uint32_t NumToSkip = Byte;
> OS << (unsigned)Byte << ", ";
> Byte = *I++;
> OS << (unsigned)Byte << ", ";
> NumToSkip |= Byte << 8;
> + Byte = *I++;
> + OS << utostr(Byte) << ", ";
> + NumToSkip |= Byte << 16;
> OS << "// Skip to: " << ((I - Table.begin()) + NumToSkip) << "\n";
> break;
> }
> @@ -765,13 +771,16 @@ void FixedLenDecoderEmitter::emitTable(f
> for (; *I >= 128; ++I)
> OS << (unsigned)*I << ", ";
> OS << (unsigned)*I++ << ", ";
> - // 16-bit numtoskip value.
> + // 24-bit numtoskip value.
> uint8_t Byte = *I++;
> uint32_t NumToSkip = Byte;
> OS << (unsigned)Byte << ", ";
> Byte = *I++;
> OS << (unsigned)Byte << ", ";
> NumToSkip |= Byte << 8;
> + Byte = *I++;
> + OS << utostr(Byte) << ", ";
> + NumToSkip |= Byte << 16;
> OS << "// Skip to: " << ((I - Table.begin()) + NumToSkip) << "\n";
> break;
> }
> @@ -782,13 +791,16 @@ void FixedLenDecoderEmitter::emitTable(f
> OS << (unsigned)*I << ", ";
> OS << (unsigned)*I++ << ", ";
>
> - // 16-bit numtoskip value.
> + // 24-bit numtoskip value.
> uint8_t Byte = *I++;
> uint32_t NumToSkip = Byte;
> OS << (unsigned)Byte << ", ";
> Byte = *I++;
> OS << (unsigned)Byte << ", ";
> NumToSkip |= Byte << 8;
> + Byte = *I++;
> + OS << utostr(Byte) << ", ";
> + NumToSkip |= Byte << 16;
> OS << "// Skip to: " << ((I - Table.begin()) + NumToSkip) << "\n";
> break;
> }
> @@ -797,7 +809,7 @@ void FixedLenDecoderEmitter::emitTable(f
> bool IsTry = *I == MCD::OPC_TryDecode;
> ++I;
> // Extract the ULEB128 encoded Opcode to a buffer.
> - uint8_t Buffer[8], *p = Buffer;
> + uint8_t Buffer[16], *p = Buffer;
> while ((*p++ = *I++) >= 128)
> assert((p - Buffer) <= (ptrdiff_t)sizeof(Buffer)
> && "ULEB128 value too large!");
> @@ -822,13 +834,16 @@ void FixedLenDecoderEmitter::emitTable(f
>
> // Fallthrough for OPC_TryDecode.
>
> - // 16-bit numtoskip value.
> + // 24-bit numtoskip value.
> uint8_t Byte = *I++;
> uint32_t NumToSkip = Byte;
> OS << (unsigned)Byte << ", ";
> Byte = *I++;
> OS << (unsigned)Byte << ", ";
> NumToSkip |= Byte << 8;
> + Byte = *I++;
> + OS << utostr(Byte) << ", ";
> + NumToSkip |= Byte << 16;
>
> OS << "// Opcode: "
> << NumberedInstructions[Opc]->TheDef->getName()
> @@ -1226,6 +1241,7 @@ void FilterChooser::emitPredicateTableEn
> TableInfo.FixupStack.back().push_back(TableInfo.Table.size());
> TableInfo.Table.push_back(0);
> TableInfo.Table.push_back(0);
> + TableInfo.Table.push_back(0);
> }
>
> void FilterChooser::emitSoftFailTableEntry(DecoderTableInfo &TableInfo,
> @@ -1311,18 +1327,19 @@ void FilterChooser::emitSingletonTableEn
> TableInfo.Table.push_back(MCD::OPC_CheckField);
> TableInfo.Table.push_back(StartBits[I-1]);
> TableInfo.Table.push_back(NumBits);
> - uint8_t Buffer[8], *p;
> + uint8_t Buffer[16], *p;
> encodeULEB128(FieldVals[I-1], Buffer);
> for (p = Buffer; *p >= 128 ; ++p)
> TableInfo.Table.push_back(*p);
> TableInfo.Table.push_back(*p);
> // Push location for NumToSkip backpatching.
> TableInfo.FixupStack.back().push_back(TableInfo.Table.size());
> - // The fixup is always 16-bits, so go ahead and allocate the space
> + // The fixup is always 24-bits, so go ahead and allocate the space
> // in the table so all our relative position calculations work OK even
> // before we fully resolve the real value here.
> TableInfo.Table.push_back(0);
> TableInfo.Table.push_back(0);
> + TableInfo.Table.push_back(0);
> }
>
> // Check for soft failure of the match.
> @@ -1342,7 +1359,7 @@ void FilterChooser::emitSingletonTableEn
> // can decode it.
> TableInfo.Table.push_back(HasCompleteDecoder ? MCD::OPC_Decode :
> MCD::OPC_TryDecode);
> - uint8_t Buffer[8], *p;
> + uint8_t Buffer[16], *p;
> encodeULEB128(Opc, Buffer);
> for (p = Buffer; *p >= 128 ; ++p)
> TableInfo.Table.push_back(*p);
> @@ -1362,6 +1379,7 @@ void FilterChooser::emitSingletonTableEn
> // Allocate the space for the fixup.
> TableInfo.Table.push_back(0);
> TableInfo.Table.push_back(0);
> + TableInfo.Table.push_back(0);
> }
> }
>
> @@ -2097,9 +2115,10 @@ static void emitDecodeInstruction(format
> << " unsigned Len;\n"
> << " InsnType Val = decodeULEB128(++Ptr, &Len);\n"
> << " Ptr += Len;\n"
> - << " // NumToSkip is a plain 16-bit integer.\n"
> + << " // NumToSkip is a plain 24-bit integer.\n"
> << " unsigned NumToSkip = *Ptr++;\n"
> << " NumToSkip |= (*Ptr++) << 8;\n"
> + << " NumToSkip |= (*Ptr++) << 16;\n"
> << "\n"
> << " // Perform the filter operation.\n"
> << " if (Val != CurFieldValue)\n"
> @@ -2120,9 +2139,10 @@ static void emitDecodeInstruction(format
> << " // Decode the field value.\n"
> << " uint32_t ExpectedValue = decodeULEB128(++Ptr, &Len);\n"
> << " Ptr += Len;\n"
> - << " // NumToSkip is a plain 16-bit integer.\n"
> + << " // NumToSkip is a plain 24-bit integer.\n"
> << " unsigned NumToSkip = *Ptr++;\n"
> << " NumToSkip |= (*Ptr++) << 8;\n"
> + << " NumToSkip |= (*Ptr++) << 16;\n"
> << "\n"
> << " // If the actual and expected values don't match, skip.\n"
> << " if (ExpectedValue != FieldValue)\n"
> @@ -2143,9 +2163,10 @@ static void emitDecodeInstruction(format
> << " // Decode the Predicate Index value.\n"
> << " unsigned PIdx = decodeULEB128(++Ptr, &Len);\n"
> << " Ptr += Len;\n"
> - << " // NumToSkip is a plain 16-bit integer.\n"
> + << " // NumToSkip is a plain 24-bit integer.\n"
> << " unsigned NumToSkip = *Ptr++;\n"
> << " NumToSkip |= (*Ptr++) << 8;\n"
> + << " NumToSkip |= (*Ptr++) << 16;\n"
> << " // Check the predicate.\n"
> << " bool Pred;\n"
> << " if (!(Pred = checkDecoderPredicate(PIdx, Bits)))\n"
> @@ -2185,9 +2206,10 @@ static void emitDecodeInstruction(format
> << " Ptr += Len;\n"
> << " unsigned DecodeIdx = decodeULEB128(Ptr, &Len);\n"
> << " Ptr += Len;\n"
> - << " // NumToSkip is a plain 16-bit integer.\n"
> + << " // NumToSkip is a plain 24-bit integer.\n"
> << " unsigned NumToSkip = *Ptr++;\n"
> << " NumToSkip |= (*Ptr++) << 8;\n"
> + << " NumToSkip |= (*Ptr++) << 16;\n"
> << "\n"
> << " // Perform the decode operation.\n"
> << " MCInst TmpMI;\n"
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
More information about the llvm-commits
mailing list