[PATCH] D88134: [LLVM-TablGen] Incorrect base type for field 'Classes' of 'struct MatchEntry' in <Target>GenAsmMatcher.inc when AsmMatcherInfo.Classes size > 252 in TableGen

[li.zhou via Phabricator via llvm-commits]

tinytinymelon created this revision.
tinytinymelon added a project: LLVM.
Herald added subscribers: llvm-commits, dexonsmith, arichardson.
tinytinymelon requested review of this revision.

**Backgroud:**

- TableGen generates `enum MatchClassKind` in <Target>GenAsmMathcer.inc to show  kinds of classes which participate in instruction matching.

  /// MatchClassKind - The kinds of classes which participate in
  /// instruction matching.
  enum MatchClassKind {
    InvalidMatchClass = 0,
    OptionalMatchClass = 1,
    MCK__DOT_1, // '.1'
    MCK__DOT_2, // '.2'
    MCK__DOT_3, // '.3'
  ...
    NumMatchClassKinds
  };



- field `Classes` of `struct MatchEntry` in same file is used to store the `enum MatchClassKind`

  struct MatchEntry {
    uint16_t Mnemonic;
    uint16_t Opcode;
    uint16_t ConvertFn;
    uint8_t RequiredFeatures;
    uint8_t Classes[13];
    ...
  };



- the base type of field `Classes` is calculated by function `getMinimalTypeForRange` in file `AsmMatcherEmitter.cpp`:

  ...
    OS << "    " << getMinimalTypeForRange(
                        std::distance(Info.Classes.begin(), Info.Classes.end()))
       << " Classes[" << MaxNumOperands << "];\n";
  ...



- logic of function `getMinimalTypeForRange` shows that if the Range <= 255, it should use `uint8_t`:

  c++
  const char *llvm::getMinimalTypeForRange(uint64_t Range, unsigned MaxSize LLVM_ATTRIBUTE_UNUSED) {
    // TODO: The original callers only used 32 and 64 so these are the only
    //       values permitted. Rather than widen the supported values we should
    //       allow 64 for the callers that currently use 32 and remove the
    //       argument altogether.
    assert((MaxSize == 32 || MaxSize == 64) && "Unexpected size");
    assert(MaxSize <= 64 && "Unexpected size");
    assert(((MaxSize > 32) ? Range <= 0xFFFFFFFFFFFFFFFFULL
                           : Range <= 0xFFFFFFFFULL) &&
           "Enum too large");
  
    if (Range > 0xFFFFFFFFULL)
      return "uint64_t";
    if (Range > 0xFFFF)
      return "uint32_t";
    if (Range > 0xFF)
      return "uint16_t";
    return "uint8_t";
  }

**Issue Description**

When decide the base type of field 'Classes', which should be decided by the size of `enum MatchClassKind` indeed, TablGen only consider the Classes defined in the `AsmMatcherInfo.Classes`, but forget the fixed header and tail enum value :

- InvalidMatchClass = 0
- OptionalMatchClass = 1
- NumMatchClassKinds

In such case, when the `AsmMatcherInfo.Classes` is larger than 252, the enum size is larger than 255 which should be represented by `uint16_t`. But the logic in file AsmMatcherEmitter.cpp gives `uint8_t` since it only check the size of  `AsmMatcherInfo.Classes`.

It will bring error message as :

> error: narrowing conversion of '(<unnamed>::MatchClassKind)256u' from 'unsigned int' to 'uin8_t {aka unsigned char}' inside {  } [-Wnarrowing]

Some of Classes with enum value > 255 will be also lost which brings to potential problems.

**Solution**

Take the 3 extra enum value in to consider when calculate the based type of field `Classes` in `struct MapEntry` as following shows:

  ...
    OS << "    " << getMinimalTypeForRange(
                        std::distance(Info.Classes.begin(), Info.Classes.end()) + 3)
       << " Classes[" << MaxNumOperands << "];\n";
  ...

It removes the error message.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88134

Files:
  utils/TableGen/AsmMatcherEmitter.cpp


Index: utils/TableGen/AsmMatcherEmitter.cpp
===================================================================
--- utils/TableGen/AsmMatcherEmitter.cpp
+++ utils/TableGen/AsmMatcherEmitter.cpp
@@ -2844,7 +2844,8 @@
   OS << "    " << getMinimalTypeForRange(MaxMask)
                << " OperandMask;\n";
   OS << "    " << getMinimalTypeForRange(std::distance(
-                      Info.Classes.begin(), Info.Classes.end())) << " Class;\n";
+                      Info.Classes.begin(), Info.Classes.end()) + 3) 
+               << " Class;\n";
   OS << "    " << getMinimalTypeForRange(MaxFeaturesIndex)
                << " RequiredFeaturesIdx;\n\n";
   OS << "    StringRef getMnemonic() const {\n";
@@ -3402,7 +3403,7 @@
   OS << "    " << getMinimalTypeForRange(FeatureBitsets.size())
                << " RequiredFeaturesIdx;\n";
   OS << "    " << getMinimalTypeForRange(
-                      std::distance(Info.Classes.begin(), Info.Classes.end()))
+                      std::distance(Info.Classes.begin(), Info.Classes.end()) + 3)
      << " Classes[" << MaxNumOperands << "];\n";
   OS << "    StringRef getMnemonic() const {\n";
   OS << "      return StringRef(MnemonicTable + Mnemonic + 1,\n";


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88134.293647.patch
Type: text/x-patch
Size: 1212 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200923/fc2d5ad7/attachment.bin>