This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by tinytinymelon on Sep 22 2020, 10:59 PM.

Details

Reviewers
stoklund
Summary

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 represent 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.

Diff Detail

Event Timeline

tinytinymelon created this revision.Sep 22 2020, 10:59 PM
tinytinymelon requested review of this revision.Sep 22 2020, 10:59 PM
tinytinymelon edited the summary of this revision. (Show Details)