This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Fix semantic discrepancy for MVT::LAST_VALUETYPE
ClosedPublic

Authored by gchatelet on May 27 2021, 7:27 AM.

Details

Summary

This patch fixes the discrepancy between all MVT::LAST_XXX enum values.

Usually LAST_XXX values are inclusive (see LAST_INTEGER_VALUETYPE or LAST_FP_VALUETYPE) but LAST_VALUETYPE behaves differently and points past the last value (See https://github.com/llvm/llvm-project/blob/438cf5577e720f84d493a272c5a1cbaf6ce19e51/llvm/include/llvm/Support/MachineValueType.h#L32)

The discrepancy is easy to spot in the _valuetypes() function at the bottom of the file
https://github.com/llvm/llvm-project/blob/438cf5577e720f84d493a272c5a1cbaf6ce19e51/llvm/include/llvm/Support/MachineValueType.h#L1370

This patch makes LAST_VALUETYPE point to the last value (as per definition) and introduces a special enum to be used as the size of a table to hold all enums up to - and including - LAST_VALUETYPE.

The patch is a bit verbose because of formatting issues the Linter was complaining about.

Diff Detail

Event Timeline

gchatelet created this revision.May 27 2021, 7:27 AM
gchatelet requested review of this revision.May 27 2021, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 7:27 AM
gchatelet added inline comments.May 27 2021, 7:30 AM
llvm/include/llvm/Support/MachineValueType.h
39

Unfortunately the Linter forced me to reformat this whole section.

262–263

The gist of this patch is the modification of these two lines.

gchatelet edited the summary of this revision. (Show Details)Jun 7 2021, 2:11 AM
courbet accepted this revision.Jun 7 2021, 2:46 AM
courbet added inline comments.
llvm/include/llvm/Support/MachineValueType.h
1373

Why not VALUETYPE_SIZE ?

This revision is now accepted and ready to land.Jun 7 2021, 2:46 AM
gchatelet marked an inline comment as done.Jun 7 2021, 3:02 AM
gchatelet added inline comments.
llvm/include/llvm/Support/MachineValueType.h
1373

For consistency with the functions below.
I have a follow up patch that will simplify all these functions so it's simpler if they look the same.

This revision was landed with ongoing or failed builds.Jun 7 2021, 3:04 AM
This revision was automatically updated to reflect the committed changes.
gchatelet marked an inline comment as done.
craig.topper added inline comments.
llvm/include/llvm/Support/MachineValueType.h
39

I really would have rather put clang-format off/on directives around this. We've been manually formatting this for a very long time because it makes it much easier to see and increment the numbers when new types are added.

gchatelet added inline comments.Jun 8 2021, 12:31 AM
llvm/include/llvm/Support/MachineValueType.h
39

Sorry for the inconvenience. I'll work on a fix and report back here.