Add support for parsing HWDiv features using target parser

[labrinea]

Summary

*To be tested by clang.

[rengolin]

There are two changes here: parse hwdiv and default hdiv.

The former looks good, the latter, not so much.

Please separate the patch in two and re-submit here only the parsing.

The default hwdiv will have to be implemented together with the other default flags, as we discussed earlier.

lib/Support/TargetParser.cpp

369

oops

[rengolin]

So, overall looks ok.

AFAICT, these values are never used as is, to use in build attributes or somewhere else, so it should be safe to change their values to what we want.

Right now, the only uses of these are either to parse (string->ID) in Clang and the assembler and ID->string in ARMAsmPrinter.cpp, or to map them to build attribute flags in ARMAsmParser.cpp.

I don't know what default value you'd put on "idiv", but I'm guessing AEK_HWDIV would be the best choice.

cheers,
--renato

include/llvm/Support/TargetParser.h

137–141

I'd put these ones at the end of the 32-bit range.

142

you don't need AEK_LAST

lib/Support/TargetParser.cpp

124

you can't change this. This is the string that is supported by the command line and the assembler.

The extensions supported by -mcpu here:

https://sourceware.org/binutils/docs/as/ARM-Options.html

are the same supported by the .arch_extension assembler directive.

If this code passed the Clang tests, it means there aren't enough tests in there for these flags.

[labrinea]

changed according to reviewer comments

[labrinea]

In D11457#212444, @rengolin wrote:

I don't know what default value you'd put on "idiv", but I'm guessing AEK_HWDIV would be the best choice.

ACLE 2.0 says __ARM_FEATURE_IDIV should be defined when both arm and thumb divide are supported. That's why I set the default value to (ARM::AEK_HWDIVARM | ARM::AEK_HWDIV)

[rengolin]

lib/Support/TargetParser.cpp

123

Let's keep these two out of it.

This is a compatibility issue and we don't want to create more unnecessary divergence than we already have.

This string will be parsed as "-mcpu=cpuname+idiv", but we already have the alternative (non-compatible) "-mhwdiv=arm/thumb", which already does what we need.

287

Couldn't you just do something like:

if (HWDivKind & ARM::AEK_HWDIV)
  Features.push_back("+hwdiv");
else
  Features.push_back("-hwdiv");

if (HWDivKind & ARM::AEK_HWDIVARM)
  Features.push_back("+hwdiv-arm");
else
  Features.push_back("-hwdiv-arm");

[rengolin]

In D11457#212479, @labrinea wrote:

ACLE 2.0 says __ARM_FEATURE_IDIV should be defined when both arm and thumb divide are supported. That's why I set the default value to (ARM::AEK_HWDIVARM | ARM::AEK_HWDIV)

Perfect.

[labrinea]

lib/Support/TargetParser.cpp

123

The idea here was to be able to do what you suggested before http://reviews.llvm.org/D11299:

void getDefaultFeatures(Features) {

unsigned DefaultFeatures = getDefaultFeatures(CPUID);
foreach (A: ARCHExtNames)
  if (DefaultFeatures & A.ID)
    Features[A.Name] = true;

}

Clang knows how to handle "+hwdiv-arm" and "+hwdiv" but not "+idiv" (at least in ARMTargetInfo).

287

Sure.

407

ARM::AEK_LAST no longer exists. I have to change this function to be iterative. Also add an equivalent function for getting HWDivName

[rengolin]

lib/Support/TargetParser.cpp

123

That's different. +hwdiv ends up as a target-feature, and we're not parsing them.

Besides, you're not using them in this patch, so it should not be added here, now.

[labrinea]

Changed according to reviewer comments.

[rengolin]

LGTM, thanks!

[labrinea]

lib/Support/TargetParser.cpp

123

I did not commit yet. Setting the ID to (ARM::AEK_HWDIVARM | ARM::AEK_HWDIV) causes regressions in llvm/tests. I think the reason is that in file lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp, getArchExtName() is called with ARM::AEK_HWDIV as argument, table lookup returns invalid and the function returns a null string. This is just one regression (CodeGen/ARM/krait-cpu-div-attribute.ll). There are more tests failing that I have not investigated yet. Shall I set ID to ARM::AEK_HWDIV as it was initially?

[rengolin]

lib/Support/TargetParser.cpp

123

I think you can safely change ARMELFStreamer to emitArchExtension(HWDIV | HWDIVARM), since that's what it means.

[labrinea]

Fixed regressions for ARM::AEK_HWDIV in ARMELFStreamer

[rengolin]

Looks good, thanks!

[rengolin]

r243335

include/llvm/Support/TargetParser.h

enum ArchKind {
     AK_ARMV7L,
     AK_ARMV7HL,
     AK_ARMV7S,
     AK_LAST
   };
 
   // Arch extension modifiers for CPUs.
   enum ArchExtKind {
-    AEK_INVALID = 0,
-    AEK_CRC,
-    AEK_CRYPTO,
-    AEK_FP,
-    AEK_HWDIV,
-    AEK_MP,
-    AEK_SIMD,
-    AEK_SEC,
-    AEK_VIRT,
+    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,
     // Unsupported extensions.
-    AEK_OS,
-    AEK_IWMMXT,
-    AEK_IWMMXT2,
-    AEK_MAVERICK,
-    AEK_XSCALE,
-    AEK_LAST
+    AEK_OS       = 0x8000000,
+    AEK_IWMMXT   = 0x10000000,
+    AEK_IWMMXT2  = 0x20000000,
+    AEK_MAVERICK = 0x40000000,
+    AEK_XSCALE   = 0x80000000,
   };
 
   // ISA kinds.
   enum ISAKind {
     IK_INVALID = 0,
     IK_ARM,
     IK_THUMB,
     IK_AARCH64
   };
enum ProfileKind {
     PK_A,
     PK_R,
     PK_M
   };
 } // namespace ARM
 
 // Target Parsers, one per architecture.
 class ARMTargetParser {
+  static StringRef getHWDivSynonym(StringRef HWDiv);
   static StringRef getFPUSynonym(StringRef FPU);
   static StringRef getArchSynonym(StringRef Arch);
 
 public:
   static StringRef getCanonicalArchName(StringRef Arch);
 
   // Information by ID
   static const char * getFPUName(unsigned FPUKind);
   static     unsigned getFPUVersion(unsigned FPUKind);
   static     unsigned getFPUNeonSupportLevel(unsigned FPUKind);
   static     unsigned getFPURestriction(unsigned FPUKind);
   static     unsigned getDefaultFPU(StringRef CPU);
   // FIXME: This should be moved to TargetTuple once it exists
   static       bool   getFPUFeatures(unsigned FPUKind,
                                      std::vector<const char*> &Features);
+  static       bool   getHWDivFeatures(unsigned HWDivKind,
+                                       std::vector<const char*> &Features);
   static const char * getArchName(unsigned ArchKind);
   static   unsigned   getArchAttr(unsigned ArchKind);
   static const char * getCPUAttr(unsigned ArchKind);
   static const char * getSubArch(unsigned ArchKind);
   static const char * getArchExtName(unsigned ArchExtKind);
+  static const char * getHWDivName(unsigned HWDivKind);
   static const char * getDefaultCPU(StringRef Arch);
 
   // Parser
+  static unsigned parseHWDiv(StringRef HWDiv);
   static unsigned parseFPU(StringRef FPU);
   static unsigned parseArch(StringRef Arch);
   static unsigned parseArchExt(StringRef ArchExt);
   static unsigned parseCPUArch(StringRef CPU);
   static unsigned parseArchISA(StringRef Arch);
   static unsigned parseArchEndian(StringRef Arch);
   static unsigned parseArchProfile(StringRef Arch);
   static unsigned parseArchVersion(StringRef Arch);
 
 };
 
 } // namespace llvm
 
 #endif

lib/Support/TargetParser.cpp

struct {
   { "armv7l",    ARM::AK_ARMV7L,   "7-L",     "v7l",   ARMBuildAttrs::CPUArch::v7 },
   { "armv7hl",   ARM::AK_ARMV7HL,  "7-L",     "v7hl",  ARMBuildAttrs::CPUArch::v7 },
   { "armv7s",    ARM::AK_ARMV7S,   "7-S",     "v7s",   ARMBuildAttrs::CPUArch::v7 }
 };
 // List of Arch Extension names.
 // FIXME: TableGen this.
 struct {
   const char *Name;
-  ARM::ArchExtKind ID;
+  unsigned ID;
 } ARCHExtNames[] = {
-  { "invalid",  ARM::AEK_INVALID },
-  { "crc",      ARM::AEK_CRC },
-  { "crypto",   ARM::AEK_CRYPTO },
-  { "fp",       ARM::AEK_FP },
-  { "idiv",     ARM::AEK_HWDIV },
-  { "mp",       ARM::AEK_MP },
-  { "simd",     ARM::AEK_SIMD },
-  { "sec",      ARM::AEK_SEC },
-  { "virt",     ARM::AEK_VIRT },
-  { "os",       ARM::AEK_OS },
-  { "iwmmxt",   ARM::AEK_IWMMXT },
-  { "iwmmxt2",  ARM::AEK_IWMMXT2 },
-  { "maverick", ARM::AEK_MAVERICK },
-  { "xscale",   ARM::AEK_XSCALE }
+  { "invalid",   ARM::AEK_INVALID },
+  { "none",      ARM::AEK_NONE },
+  { "crc",       ARM::AEK_CRC },
+  { "crypto",    ARM::AEK_CRYPTO },
+  { "fp",        ARM::AEK_FP },
+  { "idiv",     (ARM::AEK_HWDIVARM | ARM::AEK_HWDIV) },
+  { "mp",        ARM::AEK_MP },
+  { "simd",      ARM::AEK_SIMD },
+  { "sec",       ARM::AEK_SEC },
+  { "virt",      ARM::AEK_VIRT },
+  { "os",        ARM::AEK_OS },
+  { "iwmmxt",    ARM::AEK_IWMMXT },
+  { "iwmmxt2",   ARM::AEK_IWMMXT2 },
+  { "maverick",  ARM::AEK_MAVERICK },
+  { "xscale",    ARM::AEK_XSCALE }
 };
+// List of HWDiv names (use getHWDivSynonym) and which architectural
+// features they correspond to (use getHWDivFeatures).
+// FIXME: TableGen this.
+struct {
+  const char *Name;
+  unsigned ID;
+} HWDivNames[] = {
+  { "invalid",    ARM::AEK_INVALID },
+  { "none",       ARM::AEK_NONE },
+  { "thumb",      ARM::AEK_HWDIV },
+  { "arm",        ARM::AEK_HWDIVARM },
+  { "arm,thumb", (ARM::AEK_HWDIVARM | ARM::AEK_HWDIV) }
+};
 // List of CPU names and their arches.
 // The same CPU can have multiple arches and can be default on multiple arches.
 // When finding the Arch for a CPU, first-found prevails. Sort them accordingly.
 // When this becomes table-generated, we'd probably need two tables.
 // FIXME: TableGen this.
 struct {
   const char *Name;
   ARM::ArchKind ArchID;

 unsigned ARMTargetParser::getDefaultFPU(StringRef CPU) {
   for (const auto C : CPUNames) {
     if (CPU == C.Name)
       return C.DefaultFPU;
   }
   return ARM::FK_INVALID;
 }
 
+bool ARMTargetParser::getHWDivFeatures(unsigned HWDivKind,
+                                       std::vector<const char *> &Features) {
+
+  if (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 ARMTargetParser::getFPUFeatures(unsigned FPUKind,
                                      std::vector<const char *> &Features) {
 
   if (FPUKind >= ARM::FK_LAST || FPUKind == ARM::FK_INVALID)
     return false;
 
   // fp-only-sp and d16 subtarget features are independent of each other, so we
   // must enable/disable both.
case ARM::FV_NONE:
     Features.push_back("-fp-armv8");
     break;
   }
 
   // crypto includes neon, so we handle this similarly to FPU version.
   switch (FPUNames[FPUKind].NeonSupport) {
   case ARM::NS_Crypto:
     Features.push_back("+crypto");
     break;
   case ARM::NS_Neon:
     Features.push_back("+neon");
     Features.push_back("-crypto");
     break;
   case ARM::NS_None:
     Features.push_back("-neon");
     Features.push_back("-crypto");
     break;

 }
 
 unsigned ARMTargetParser::getArchAttr(unsigned ArchKind) {
   if (ArchKind >= ARM::AK_LAST)
     return ARMBuildAttrs::CPUArch::Pre_v4;
   return ARCHNames[ArchKind].ArchAttr;
 }
 
 const char *ARMTargetParser::getArchExtName(unsigned ArchExtKind) {
-  if (ArchExtKind >= ARM::AEK_LAST)
-    return nullptr;
-  return ARCHExtNames[ArchExtKind].Name;
+  for (const auto AE : ARCHExtNames) {
+    if (ArchExtKind == AE.ID)
+      return AE.Name;
+  }
+  return nullptr;
+}
+
+const char *ARMTargetParser::getHWDivName(unsigned HWDivKind) {
+  for (const auto D : HWDivNames) {
+    if (HWDivKind == D.ID)
+      return D.Name;
+  }
 }
 
 const char *ARMTargetParser::getDefaultCPU(StringRef Arch) {
   unsigned AK = parseArch(Arch);
   if (AK == ARM::AK_INVALID)
     return nullptr;
 
   // Look for multiple AKs to find the default for pair AK+Name.
   for (const auto CPU : CPUNames) {
     if (CPU.ArchID == AK && CPU.Default)
       return CPU.Name;
   }
   return nullptr;
 }
 
 // ======================================================= //
 // Parsers
 // ======================================================= //
 
+StringRef ARMTargetParser::getHWDivSynonym(StringRef HWDiv) {
+  return StringSwitch<StringRef>(HWDiv)
+    .Case("thumb,arm", "arm,thumb")
+    .Default(HWDiv);
+}
+
 StringRef ARMTargetParser::getFPUSynonym(StringRef FPU) {
   return StringSwitch<StringRef>(FPU)
     .Cases("fpa", "fpe2", "fpe3", "maverick", "invalid") // Unsupported
     .Case("vfp2", "vfpv2")
     .Case("vfp3", "vfpv3")
     .Case("vfp4", "vfpv4")
     .Case("vfp3-d16", "vfpv3-d16")
     .Case("vfp4-d16", "vfpv4-d16")
// Must start with 'vN'.
     if (A.find("eb") != StringRef::npos)
       return Error;
   }
 
   // Arch will either be a 'v' name (v7a) or a marketing name (xscale).
   return A;
 }
 
+unsigned ARMTargetParser::parseHWDiv(StringRef HWDiv) {
+  StringRef Syn = getHWDivSynonym(HWDiv);
+  for (const auto D : HWDivNames) {
+    if (Syn == D.Name)
+      return D.ID;
+  }
+  return ARM::AEK_INVALID;
+}
+
 unsigned ARMTargetParser::parseFPU(StringRef FPU) {
   StringRef Syn = getFPUSynonym(FPU);
   for (const auto F : FPUNames) {
     if (Syn == F.Name)
       return F.ID;
   }
   return ARM::FK_INVALID;
 }

lib/Target/ARM/ARMAsmPrinter.cpp

void ARMAsmPrinter::emitAttributes() {
 
   if (CPUString.find("generic") != 0) { //CPUString doesn't start with "generic"
     // FIXME: remove krait check when GNU tools support krait cpu
     if (STI.isKrait()) {
       ATS.emitTextAttribute(ARMBuildAttrs::CPU_name, "cortex-a9");
       // We consider krait as a "cortex-a9" + hwdiv CPU
       // Enable hwdiv through ".arch_extension idiv"
       if (STI.hasDivide() || STI.hasDivideInARMMode())
-        ATS.emitArchExtension(ARM::AEK_HWDIV);
+        ATS.emitArchExtension(ARM::AEK_HWDIV | ARM::AEK_HWDIVARM);
     } else
       ATS.emitTextAttribute(ARMBuildAttrs::CPU_name, CPUString);
   }
 
   ATS.emitAttribute(ARMBuildAttrs::CPU_arch, getArchForCPU(CPUString, &STI));
 
   // Tag_CPU_arch_profile must have the default value of 0 when "Architecture
   // profile is not applicable (e.g. pre v7, or cross-profile code)".

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

 #define GET_SUBTARGET_FEATURE_NAME
 #define GET_MATCHER_IMPLEMENTATION
 #include "ARMGenAsmMatcher.inc"
 
 // FIXME: This structure should be moved inside ARMTargetParser
 // when we start to table-generate them, and we can use the ARM
 // flags below, that were generated by table-gen.
 static const struct {
-  const ARM::ArchExtKind Kind;
+  const unsigned Kind;
   const unsigned ArchCheck;
   const FeatureBitset Features;
 } Extensions[] = {
   { ARM::AEK_CRC, Feature_HasV8, {ARM::FeatureCRC} },
   { ARM::AEK_CRYPTO,  Feature_HasV8,
     {ARM::FeatureCrypto, ARM::FeatureNEON, ARM::FeatureFPARMv8} },
   { ARM::AEK_FP, Feature_HasV8, {ARM::FeatureFPARMv8} },
-  { ARM::AEK_HWDIV, Feature_HasV7 | Feature_IsNotMClass,
+  { (ARM::AEK_HWDIV | ARM::AEK_HWDIVARM), Feature_HasV7 | Feature_IsNotMClass,
     {ARM::FeatureHWDiv, ARM::FeatureHWDivARM} },
   { ARM::AEK_MP, Feature_HasV7 | Feature_IsNotMClass, {ARM::FeatureMP} },
   { ARM::AEK_SIMD, Feature_HasV8, {ARM::FeatureNEON, ARM::FeatureFPARMv8} },
   // FIXME: Also available in ARMv6-K
   { ARM::AEK_SEC, Feature_HasV7, {ARM::FeatureTrustZone} },
   // FIXME: Only available in A-class, isel not predicated
   { ARM::AEK_VIRT, Feature_HasV7, {ARM::FeatureVirtualization} },
   // FIXME: Unsupported extensions.