This is an archive of the discontinued LLVM Phabricator instance.

[TargetParser] Use enum classes for various ARM kind enums.
ClosedPublic

Authored by fhahn on Jul 26 2017, 2:29 AM.

Details

Summary

Using c++11 enum classes ensures that only valid enum values are used
for ArchKind, ProfileKind, VersionKind and ISAKind. This removes the
need for checks that the provided values map to a proper enum value,
allows us to get rid of AK_LAST and prevents comparing values from
different enums. It also removes a bunch of static_cast
from unsigned to enum values and vice versa, at the cost of introducing
static casts to access AArch64ARCHNames and ARMARCHNames by ArchKind.

FPUKind and ArchExtKind are the only remaining old-style enum in
TargetParser.h. I think it's beneficial to keep ArchExtKind as old-style
enum, but FPUKind can be converted too, but this patch is quite big, so
could do this in a follow-up patch. I could also split this patch up a
bit, if people would prefer that.

Diff Detail

Event Timeline

fhahn created this revision.Jul 26 2017, 2:29 AM
fhahn added a comment.Jul 26 2017, 2:29 AM

This commit should not change any existing functionality, but I think technically it's not really a NFC, as it restricts the APIs of the functions taking various enum values as unsigned previously.

javed.absar edited edge metadata.Jul 26 2017, 8:27 PM

Looks like a nice clean-up.
Its a pity we still need 'invalid' despite using enum class.

LGTM, but will wait for others to comment.

Thanks Javed! I think we may be able to get rid of INVALID, after this change goes in, by doing the validation early once. I'll try to look into this after this one went in.

fhahn added a reviewer: rovka.Jul 27 2017, 2:00 AM
rovka added inline comments.Jul 27 2017, 5:15 AM
lib/Support/TargetParser.cpp
380

This can still return INVALID, I don't think you should remove the check just yet.

788

I'd rather make the switch fully covered than add a default. Also, you could add an unreachable before the end of the function.

unittests/Support/TargetParserTest.cpp
452–454

Are the extra checks necessary?

789

This kind of iteration was sort of ok when there was an AK_LAST, but I think with that gone there's the chance that this test won't get updated when someone adds something after ARMV8_2A.

One alternative would be to add the enum values to a vector (you can just #define AARCH64_ARCH(NAME, ID [...]) to push_back and include AArch64TargetParser.def) and iterate over the vector instead.

fhahn updated this revision to Diff 108454.Jul 27 2017, 6:26 AM
fhahn marked an inline comment as done.

Thanks for having a look Diana. I hope I have addressed your comments adequately

fhahn added inline comments.Jul 27 2017, 6:27 AM
lib/Support/TargetParser.cpp
380

I thought a change in the .def file made that unnecessary, but I was mistaken.

788

Excellent point! I've removed the default case from this switch and the switch above in ARM::parseArchProfile.

unittests/Support/TargetParserTest.cpp
452–454

Unfortunately yes, but I am not sure if that test adds much value.

It seems like the previous test just checked if "0 <= ARM::getFPUVersion(FK)" for all FK holds. I think the intention was to check that it always returns an element in the enum, but wouldn't the condition be always true, because ARM::getFPUVersion(FK) used to return unsigned?

I think the tests in ARMFPUNeonSupportLevel and ARMFPURestriction have the same problem.

789

Agreed, thanks for the vector tip! :)

rovka accepted this revision.Jul 27 2017, 7:45 AM

LGTM.

unittests/Support/TargetParserTest.cpp
452–454

You're right, these tests were a bit iffy to begin with.

789

That's even nicer :)

This revision is now accepted and ready to land.Jul 27 2017, 7:45 AM
fhahn added a comment.Jul 27 2017, 7:50 AM

Great thanks again for having a look! This patch needs to go in with D35884, so I'll wait until D35884 is reviewed to and commit them together.

fhahn closed this revision.Jul 27 2017, 9:28 AM