This is an archive of the discontinued LLVM Phabricator instance.

[mlir] EnumsGen: dissociate string form of integer enum from C++ symbol name
ClosedPublic

Authored by ftynse on Jan 24 2020, 8:52 AM.

Details

Summary

In some cases, one may want to use different names for C++ symbol of an
enumerand from its string representation. In particular, in the LLVM dialect
for, e.g., Linkage, we would like to preserve the same enumerand names as LLVM
API and the same textual IR form as LLVM IR, yet the two are different
(CamelCase vs snake_case with additional limitations on not being a C++
keyword).

Modify EnumAttrCaseInfo in OpBase.td to include both the integer value and its
string representation. By default, this representation is the same as C++
symbol name. Introduce new IntStrAttrCaseBase that allows one to use different
names. Exercise it for LLVM Dialect Linkage attribute. Other attributes will
follow as separate changes.

Diff Detail

Event Timeline

ftynse created this revision.Jan 24 2020, 8:52 AM
ftynse added a subscriber: flaub.

Unit tests: fail. 62054 tests passed, 5 failed and 783 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Thanks Alex for adding this! Have you considered adding an additional parameter (defaulting to C++ enumerant symbol) for the string representation instead of duplicating all the cases? I can see this is also useful for BitEnumAttrCase and StrEnumAttrCase so it ends up we need to duplicate all the cases. Besides, the name of IntStrEnumCase can be quite confusing, given we have IntEnumCase and StrEnumCase (and in the future may have StrStrEnumCase/BitStrEnumCase).

flaub added a comment.Jan 24 2020, 3:48 PM

This is great, I can update the recent LLVM dialect enums that I added to use this. Thanks!

Thanks Alex for adding this! Have you considered adding an additional parameter (defaulting to C++ enumerant symbol) for the string representation instead of duplicating all the cases?

I'm not sure I quite understand. I did add an additional parameter to EnumAttrCaseInfo and it is defaulted to C++ enumerant symbol for StrEnumAttrCase and for I32EnumAttrCase/I64EnumAttrCase that are currently in use. It made more sense to me to have this value at the top-level class exactly because it could be useful for other kinds of attributes. I could not have IntEnumAttrCaseBase that inherits from IntStrEnumAttrCaseBase for the purposes of defaulting the string representation to C++ enumerant since, in this case, I32EnumAttr and I64EnumAttr would have to be duplicated instead. They accept I32/64StrEnumAttrCase so can also accept the derived I32/64EnumAttrCase now, but it would not be correct if the defaulting happened before the type is specified.

I can see this is also useful for BitEnumAttrCase and StrEnumAttrCase so it ends up we need to duplicate all the cases. Besides, the name of IntStrEnumCase can be quite confusing, given we have IntEnumCase and StrEnumCase (and in the future may have StrStrEnumCase/BitStrEnumCase).

How about AnnotatedIntEnumCase ?

I'm not sure I quite understand. I did add an additional parameter to EnumAttrCaseInfo and it is defaulted to C++ enumerant symbol for StrEnumAttrCase and for I32EnumAttrCase/I64EnumAttrCase that are currently in use. It made more sense to me to have this value at the top-level class exactly because it could be useful for other kinds of attributes. I could not have IntEnumAttrCaseBase that inherits from IntStrEnumAttrCaseBase for the purposes of defaulting the string representation to C++ enumerant since, in this case, I32EnumAttr and I64EnumAttr would have to be duplicated instead. They accept I32/64StrEnumAttrCase so can also accept the derived I32/64EnumAttrCase now, but it would not be correct if the defaulting happened before the type is specified.

I meant that instead of having both I32EnumAttrCase (taking two parameters) and I32StrEnumAttrCase (taking three parameters), what about just having I32EnumAttrCase and making it taking in string cSym, int cVal, string strVal = cSym? Similarly for other *EnumAttrCases.

ftynse updated this revision to Diff 241084.Jan 29 2020, 2:55 AM

Use default value for parameter and drop IntStr*

I meant that instead of having both I32EnumAttrCase (taking two parameters) and I32StrEnumAttrCase (taking three parameters), what about just having I32EnumAttrCase and making it taking in string cSym, int cVal, string strVal = cSym? Similarly for other *EnumAttrCases.

Great suggestion, thanks! Done.

Unit tests: pass. 62286 tests passed, 0 failed and 831 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

antiagainst accepted this revision.Jan 30 2020, 7:21 AM

Nice, thanks Alex for adding this!

This revision was not accepted when it landed; it landed in state Needs Review.Jan 30 2020, 8:04 AM
This revision was automatically updated to reflect the committed changes.