Ajust two tests implementation of TargetParserTest

[jojo]

Summary

The loop from 0 ~ AEK_XSCALE(0x80000000) in these tests will over the entire int range.
Actually,the ArchExtKind from AEK_OS(0x8000000) to AEK_XSCALE(0x80000000) are unsupported extensions for now.
So do not care about these extensions in the tests of TargetParserTest.
If these extensions will be supported in the future,they would be given a reasonable value absolutely.

[djasper]

I am not the right reviewer here. I still believe looping over 0 to 0x8000000 is not useful and actually takes a lot of time in non-optimized builds. Is there no better way? What are you actually trying to test?

[jojo]

It's possible to keep them included in the tests,just remove them from the main loop.
But I think it's meaningless,as they are unsupported in the design.
@rengolin,Could you give me some advice？

[rengolin]

unittests/Support/TargetParserTest.cpp

217

You could iterate over kHWDivKinds and kARMArchExtKinds?

218

Shouldn't this be "< AEK_LAST"?

[jojo]

three choices:

1. Just iterate to AEK_RAS, as shown in this diff. (smallest change)

As AEK_RAS in the enum is the last one supported in the current design,so iterate to it may seems odd but is enough.

2. Add option AEK_LAST to enum ArchExtKind,then iterate to AEK_LAST.(biggest change, I prefer this one)

The current definition of ArchExtKind is as follow,adding an end option like FPUKind and ArchKind maybe better. Update diff
for this one.

enum ArchExtKind : unsigned {
  AEK_INVALID = 0x0,
  AEK_NONE = 0x1,
  AEK_CRC = 0x2,
  AEK_CRYPTO = 0x4,
  AEK_FP = 0x8,
  AEK_HWDIV = 0x10,
  AEK_HWDIVARM = 0x20,
  AEK_MP = 0x40,
  AEK_SIMD = 0x80,
  AEK_SEC = 0x100,
  AEK_VIRT = 0x200,
  AEK_DSP = 0x400,
  AEK_FP16 = 0x800,
  AEK_RAS = 0x1000,
  // Unsupported extensions.
  AEK_OS = 0x8000000,
  AEK_IWMMXT = 0x10000000,
  AEK_IWMMXT2 = 0x20000000,
  AEK_MAVERICK = 0x40000000,
  AEK_XSCALE = 0x80000000,
};

3. iterate over kHWDivKinds and kARMArchExtKinds

@rengolin,What is your opinion？

[rengolin]

In D22956#501906, @jojo wrote:

1. Just iterate to AEK_RAS, as shown in this diff. (smallest change)

As AEK_RAS in the enum is the last one supported in the current design,so iterate to it may seems odd but is enough.

It is odd, and error prone, if we re-order or change things.

2. Add option AEK_LAST to enum ArchExtKind,then iterate to AEK_LAST.(biggest change, I prefer this one)

Me too. But I'm still not convinced the two additional tests (ARMArchExtName, ARMHWDivName) add anything of value.

If we make a mistake in the enum, the test will still pass, since both the enum and the test array are generated from the same source (the .def file).

We're already testing the extensions in ARMArchExtFeature, so why not just leave it at that?

cheers,
--renato

include/llvm/Support/TargetParser.h

93

AEK_LAST seems like a bad name for this... Maybe AEK_LAST_SUPPORTED?

Also, best if put in order, ie. after AEK_RAS.

lib/Support/TargetParser.cpp

205

This encodes the wrong information. This is telling you that *no* unsupported arch has HDIV, when this might not be true.

Why do you need to change this at all?

[jojo]

2. Add option AEK_LAST to enum ArchExtKind,then iterate to AEK_LAST.(biggest change, I prefer this one)

Me too. But I'm still not convinced the two additional tests (ARMArchExtName, ARMHWDivName) add anything of value.

If we make a mistake in the enum, the test will still pass, since both the enum and the test array are generated from the same source (the .def file).

We're already testing the extensions in ARMArchExtFeature, so why not just leave it at that?

Indeed, agree.
For now it's ok leave it at that. Add additional test cases when there is really a need later on.
In a more reasonable way of course.

Thanks renato and djasper!

include/llvm/Support/TargetParser.h

enum ArchExtKind : unsigned {
   AEK_FP16 = 0x800,
   AEK_RAS = 0x1000,
   // Unsupported extensions.
   AEK_OS = 0x8000000,
   AEK_IWMMXT = 0x10000000,
   AEK_IWMMXT2 = 0x20000000,
   AEK_MAVERICK = 0x40000000,
   AEK_XSCALE = 0x80000000,
+  AEK_LAST = 0x2000
 };
 
 // ISA kinds.
 enum ISAKind { IK_INVALID = 0, IK_ARM, IK_THUMB, IK_AARCH64 };
 
 // Endianness
 // FIXME: BE8 vs. BE32?
 enum EndianKind { EK_INVALID = 0, EK_LITTLE, EK_BIG };
enum ArchExtKind : unsigned {
   AEK_INVALID = 0x0,
   AEK_NONE = 0x1,
   AEK_CRC = 0x2,
   AEK_CRYPTO = 0x4,
   AEK_FP = 0x8,
   AEK_SIMD = 0x10,
   AEK_FP16 = 0x20,
   AEK_PROFILE = 0x40,
-  AEK_RAS = 0x80
+  AEK_RAS = 0x80,
+  AEK_LAST = 0x100
 };
 
 StringRef getCanonicalArchName(StringRef Arch);
 
 // Information by ID
 StringRef getFPUName(unsigned FPUKind);
 unsigned getFPUVersion(unsigned FPUKind);
 unsigned getFPUNeonSupportLevel(unsigned FPUKind);

lib/Support/TargetParser.cpp

#define ARM_CPU_NAME(NAME, ID, DEFAULT_FPU, IS_DEFAULT, DEFAULT_EXT) \
     .Case(NAME, ARCHNames[ID].ArchBaseExtensions | DEFAULT_EXT)
 #include "llvm/Support/ARMTargetParser.def"
     .Default(ARM::AEK_INVALID);
 }
 
 bool llvm::ARM::getHWDivFeatures(unsigned HWDivKind,
                                  std::vector<const char *> &Features) {
 
-  if (HWDivKind == ARM::AEK_INVALID)
+  if (HWDivKind >= ARM::AEK_LAST || HWDivKind == ARM::AEK_INVALID)
     return false;
 
   if (HWDivKind & ARM::AEK_HWDIVARM)
     Features.push_back("+hwdiv-arm");
   else
     Features.push_back("-hwdiv-arm");
 
   if (HWDivKind & ARM::AEK_HWDIV)
     Features.push_back("+hwdiv");
   else
     Features.push_back("-hwdiv");
 
   return true;
 }
 
 bool llvm::ARM::getExtensionFeatures(unsigned Extensions,
                                      std::vector<const char *> &Features) {
 
-  if (Extensions == ARM::AEK_INVALID)
+  if (Extensions >= ARM::AEK_LAST || Extensions == ARM::AEK_INVALID)
     return false;
 
   if (Extensions & ARM::AEK_CRC)
     Features.push_back("+crc");
   else
     Features.push_back("-crc");
 
   if (Extensions & ARM::AEK_DSP)
#define AARCH64_CPU_NAME(NAME, ID, DEFAULT_FPU, IS_DEFAULT, DEFAULT_EXT) \
     .Case(NAME, DEFAULT_EXT)
 #include "llvm/Support/AArch64TargetParser.def"
     .Default(AArch64::AEK_INVALID);
 }
 
 bool llvm::AArch64::getExtensionFeatures(unsigned Extensions,
                                      std::vector<const char *> &Features) {
 
-  if (Extensions == AArch64::AEK_INVALID)
+  if (Extensions >= AArch64::AEK_LAST || Extensions == AArch64::AEK_INVALID)
     return false;
 
   if (Extensions & AArch64::AEK_FP)
     Features.push_back("+fp-armv8");
   if (Extensions & AArch64::AEK_SIMD)
     Features.push_back("+neon");
   if (Extensions & AArch64::AEK_CRC)
     Features.push_back("+crc");

unittests/Support/TargetParserTest.cpp

 #include "llvm/Support/ARMBuildAttributes.h"
 #include "llvm/Support/TargetParser.h"
 #include "gtest/gtest.h"
 #include <string>
 
 using namespace llvm;
 
 namespace {
+static const unsigned kHWDivKinds[] = {
+#define ARM_HW_DIV_NAME(NAME, ID) ID,
+#include "llvm/Support/ARMTargetParser.def"
+#undef ARM_HW_DIV_NAME
+};
+
+static const unsigned kARMArchExtKinds[] = {
+#define ARM_ARCH_EXT_NAME(NAME, ID, FEATURE, NEGFEATURE) ID,
+#include "llvm/Support/ARMTargetParser.def"
+#undef ARM_ARCH_EXT_NAME
+};
+
 static const unsigned kAArch64ArchExtKinds[] = {
 #define AARCH64_ARCH_EXT_NAME(NAME, ID, FEATURE, NEGFEATURE) ID,
 #include "llvm/Support/AArch64TargetParser.def"
 #undef AARCH64_ARCH_EXT_NAME
 };
 
 template <typename T> struct ArchNames {
   const char *Name;
TEST(TargetParserTest, ARMArchAttr) {
   for (ARM::ArchKind AK = static_cast<ARM::ArchKind>(0);
        AK <= ARM::ArchKind::AK_LAST;
        AK = static_cast<ARM::ArchKind>(static_cast<unsigned>(AK) + 1))
     EXPECT_TRUE(AK == ARM::AK_LAST
                     ? (ARMBuildAttrs::CPUArch::Pre_v4 == ARM::getArchAttr(AK))
                     : (kARMARCHNames[AK].ArchAttr == ARM::getArchAttr(AK)));
 }
 
+TEST(TargetParserTest, ARMArchExtName) {
+  for (ARM::ArchExtKind AEK = static_cast<ARM::ArchExtKind>(0);
+       AEK <= ARM::ArchExtKind::AEK_LAST;
+       AEK = static_cast<ARM::ArchExtKind>(static_cast<unsigned>(AEK) + 1))
+    EXPECT_TRUE(contains(kARMArchExtKinds, static_cast<unsigned>(AEK))
+                    ? !ARM::getArchExtName(AEK).empty()
+                    : ARM::getArchExtName(AEK).empty());
+}
+
 TEST(TargetParserTest, ARMArchExtFeature) {
   const char *ArchExt[][4] = {{"crc", "nocrc", "+crc", "-crc"},
                               {"crypto", "nocrypto", "+crypto", "-crypto"},
                               {"dsp", "nodsp", "+dsp", "-dsp"},
                               {"fp", "nofp", nullptr, nullptr},
                               {"idiv", "noidiv", nullptr, nullptr},
                               {"mp", "nomp", nullptr, nullptr},
                               {"simd", "nosimd", nullptr, nullptr},
                               {"sec", "nosec", nullptr, nullptr},
                               {"virt", "novirt", nullptr, nullptr},
                               {"fp16", "nofp16", "+fullfp16", "-fullfp16"},
                               {"ras", "noras", "+ras", "-ras"},
                               {"os", "noos", nullptr, nullptr},
                               {"iwmmxt", "noiwmmxt", nullptr, nullptr},
                               {"iwmmxt2", "noiwmmxt2", nullptr, nullptr},
                               {"maverick", "maverick", nullptr, nullptr},
                               {"xscale", "noxscale", nullptr, nullptr}};
 
   for (unsigned i = 0; i < array_lengthof(ArchExt); i++) {
     EXPECT_STREQ(ArchExt[i][2], ARM::getArchExtFeature(ArchExt[i][0]));
     EXPECT_STREQ(ArchExt[i][3], ARM::getArchExtFeature(ArchExt[i][1]));
   }
 }
 
+TEST(TargetParserTest, ARMHWDivName) {
+  for (ARM::ArchExtKind AEK = static_cast<ARM::ArchExtKind>(0);
+       AEK <= ARM::ArchExtKind::AEK_LAST;
+       AEK = static_cast<ARM::ArchExtKind>(static_cast<unsigned>(AEK) + 1))
+    EXPECT_TRUE(contains(kHWDivKinds, static_cast<unsigned>(AEK))
+                    ? !ARM::getHWDivName(AEK).empty()
+                    : ARM::getHWDivName(AEK).empty());
+}
+
 TEST(TargetParserTest, ARMDefaultCPU) {
   for (unsigned i = 0; i < array_lengthof(ARMArch); i++)
     EXPECT_FALSE(ARM::getDefaultCPU(ARMArch[i]).empty());
 }
 
 TEST(TargetParserTest, ARMparseHWDiv) {
   const char *hwdiv[] = {"thumb", "arm", "arm,thumb", "thumb,arm"};
 
for (unsigned AK = 0; AK < static_cast<unsigned>(AArch64::ArchKind::AK_LAST);
     EXPECT_TRUE(
         AK == static_cast<unsigned>(AArch64::ArchKind::AK_LAST)
             ? (ARMBuildAttrs::CPUArch::v8_A == AArch64::getArchAttr(AK))
             : (kAArch64ARCHNames[AK].ArchAttr == AArch64::getArchAttr(AK)));
 }
 
 TEST(TargetParserTest, AArch64ArchExtName) {
   for (AArch64::ArchExtKind AEK = static_cast<AArch64::ArchExtKind>(0);
-       AEK <= AArch64::ArchExtKind::AEK_RAS;
+       AEK <= AArch64::ArchExtKind::AEK_LAST;
        AEK = static_cast<AArch64::ArchExtKind>(static_cast<unsigned>(AEK) + 1))
     EXPECT_TRUE(contains(kAArch64ArchExtKinds, static_cast<unsigned>(AEK))
                     ? !AArch64::getArchExtName(AEK).empty()
                     : AArch64::getArchExtName(AEK).empty());
 }
 
 TEST(TargetParserTest, AArch64ArchExtFeature) {
   const char *ArchExt[][4] = {{"crc", "nocrc", "+crc", "-crc"},