[ARM] Add knowledge of FPU subtarget features to TargetParser

[john.brawn]

Summary

Add getFPUFeatures to TargetParser, which gets the list of subtarget features that are enabled/disabled for each FPU, and use it when handling the .fpu directive.

No functional change in this commit, though clang will start behaving differently once it starts using this.

[rengolin]

Hi John,

Thanks for working on this.

I think this patch could be split into smaller chunks, and move other chunks elsewhere.

One patch is to add the enums, update the table and create the get methods. No <vector>.

Another is to move the parsing logic from ARMAsmParser to a new method, one outside the ARMTargetParser and ARMAsmParser, that uses a Features vector and common both assembler's and Clang's feature bits parsing, but only changes in the assembler side (LLVM patch). ARMSubTarget is probably the best place for it.

The final patch (D10239), will then just move to this new method.

Makes sense?

cheers,
--renato

include/llvm/Support/TargetParser.h

18

This will include <vector> is a lot of other files that don't need it. Can you use SmallVector or something and declare it like StringRef?

lib/Support/TargetParser.cpp

27

I'm leaving enums in the header file for now. It may not be pretty to expose the internal of it for now, but we need to understand where things are used and why, so that when we implement the new Tuple class (see the list for more details), we can merge enums from Triple, TargetParser and others into one big, internal, implementation detail.

253

I think this method is too specific, and it should be implemented in the user's side.

The overall design of this class must be as simple as possible, even if that means more code on the user side. So, create two new methods getRestriction() and getNeonSupport() by using the two new columns provided and implement the rest in the caller's side.

257

No need for brackets here.

[john.brawn]

Another is to move the parsing logic from ARMAsmParser to a new method, one outside the ARMTargetParser and ARMAsmParser, that uses a Features vector and common both assembler's and Clang's feature bits parsing, but only changes in the assembler side (LLVM patch). ARMSubTarget is probably the best place for it.

The problem with moving that part to ARMSubTarget is that then clang can't see it, as nothing in lib/Target/TargetName is exposed in include/llvm/Target. I could maybe put something in MC/SubtargetFeature or MC/MCSubtargetInfo, but that would mean ARM-specific stuff is polluting a target-independent interface unless I do something like add a target-independent TargetParser layer on top of ARMTargetParser.

include/llvm/Support/TargetParser.h

18

std::vector is the standard way of handling lists of subtarget feature flags in clang, so using something else would make the interface with clang messier. Also there are quite a lot of headers that include <vector>, and looking at the places TargetParser is used they're probably including one of them already.

[rengolin]

What about moving it to Triple?

[john.brawn]

In D10237#183519, @rengolin wrote:

What about moving it to Triple?

That would be a bit of an odd place to do it, as FPUs and triples only indirectly have anything to do with each other - a triple may indirectly cause a default FPU due to the default CPU, but there's no link in the FPU->Triple direction.

It's not clear to my why TargetParser isn't the right place for this. You say "The overall design of this class must be as simple as possible", but I don't see why that is. My impression of TargetParser is that it handles target-specific names in a way that means the thing using it doesn't need target-specific information, e.g. you can go from architecture name to default CPU name without needing to know what those names mean. Going from FPU name to a list of subtarget feature flags doesn't seem that different to me.

[rengolin]

In D10237#183533, @john.brawn wrote:

That would be a bit of an odd place to do it, as FPUs and triples only indirectly have anything to do with each other - a triple may indirectly cause a default FPU due to the default CPU, but there's no link in the FPU->Triple direction.

That's a good point.

It's not clear to my why TargetParser isn't the right place for this. You say "The overall design of this class must be as simple as possible", but I don't see why that is. My impression of TargetParser is that it handles target-specific names in a way that means the thing using it doesn't need target-specific information, e.g. you can go from architecture name to default CPU name without needing to know what those names mean. Going from FPU name to a list of subtarget feature flags doesn't seem that different to me.

Now I'm less convinced that this doesn't belong there, but I really dislike the vector solution.

Clang uses std::vector and std::string too liberally, and I don't think that's good enough for LLVM base libraries. The argument that current cpp files already include <vector> is only valid until we start including TargetParser.h in other cpp files that don't. Standard library headers are very nasty indeed.

However, as you said, any other place will be worse than here... except the future TargetTuple class that doesn't exist yet. The new Tuple is like a Triple on steroids, that knows everything about the target, not just CPU and ARCH, but FPU and extensions, OS and vendor with the necessary and unambiguous context.

For now, I guess we can just leave it here and add a big FIXME on "include <vector>", to remove it as soon as features move to Tuple, and the method itself to move this to Tuple once that's created.

Adding the FIXMEs, removing the spurious brackets and moving the enums to the headers, LGTM. Thanks!

cheers,
--renato

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

9199

no need for brackets any more here either.

[jroelofs]

lib/Support/TargetParser.cpp

35

it would be much better to encode these comments like this:

FR_None,   //< No Restriction
FR_D16,    //< Only 16 D registers
FR_SP_D16, //< Only single precision instructions, with 16 D registers

48

and this one like this:

unsigned FPUVersion; //< corresponds directly to the FP arch version number

include/llvm/MC/MCSubtargetInfo.h

public:
   /// ToggleFeature - Toggle a feature and returns the re-computed feature
   /// bits. This version does not change the implied bits.
   FeatureBitset ToggleFeature(const FeatureBitset& FB);
 
   /// ToggleFeature - Toggle a set of features and returns the re-computed
   /// feature bits. This version will also change all implied bits.
   FeatureBitset ToggleFeature(StringRef FS);
 
+  /// Apply a feature flag and return the re-computed feature bits, including
+  /// all feature bits implied by the flag.
+  FeatureBitset ApplyFeatureFlag(StringRef FS);
+
   /// getSchedModelForCPU - Get the machine model of a CPU.
   ///
   MCSchedModel getSchedModelForCPU(StringRef CPU) const;
 
   /// getSchedModel - Get the machine model for this subtarget's CPU.
   ///
   const MCSchedModel &getSchedModel() const { return CPUSchedModel; }
 

include/llvm/MC/SubtargetFeature.h

public:
   /// Adding Features.
   void AddFeature(StringRef String, bool Enable = true);
 
   /// ToggleFeature - Toggle a feature and returns the newly updated feature
   /// bits.
   FeatureBitset ToggleFeature(FeatureBitset Bits, StringRef String,
                          ArrayRef<SubtargetFeatureKV> FeatureTable);
 
+  /// Apply the feature flag and return the newly updated feature bits.
+  FeatureBitset ApplyFeatureFlag(FeatureBitset Bits, StringRef Feature,
+                                 ArrayRef<SubtargetFeatureKV> FeatureTable);
+
   /// Get feature bits of a CPU.
   FeatureBitset getFeatureBits(StringRef CPU,
                           ArrayRef<SubtargetFeatureKV> CPUTable,
                           ArrayRef<SubtargetFeatureKV> FeatureTable);
 
   /// Print feature string.
   void print(raw_ostream &OS) const;
 

include/llvm/Support/TargetParser.h

 // This file implements a target parser to recognise hardware features such as
 // FPU/CPU/ARCH names as well as specific support such as HDIV, etc.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_TARGETPARSER_H
 #define LLVM_SUPPORT_TARGETPARSER_H
 
+#include <vector>
+
 namespace llvm {
   class StringRef;
 
 // Target specific information into their own namespaces. These should be
 // generated from TableGen because the information is already there, and there
 // is where new information about targets will be added.
 // FIXME: To TableGen this we need to make some table generated files available
 // even if the back-end is not compiled with LLVM, plus we need to create a new
class ARMTargetParser {
   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       bool   getFPUFeatures(unsigned FPUKind,
+                                     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 * getDefaultCPU(StringRef Arch);
 
   // Parser

lib/MC/MCSubtargetInfo.cpp

 /// ToggleFeature - Toggle a feature and returns the re-computed feature
 /// bits. This version will also change all implied bits.
 FeatureBitset MCSubtargetInfo::ToggleFeature(StringRef FS) {
   SubtargetFeatures Features;
   FeatureBits = Features.ToggleFeature(FeatureBits, FS, ProcFeatures);
   return FeatureBits;
 }
 
+FeatureBitset MCSubtargetInfo::ApplyFeatureFlag(StringRef FS) {
+  SubtargetFeatures Features;
+  FeatureBits = Features.ApplyFeatureFlag(FeatureBits, FS, ProcFeatures);
+  return FeatureBits;
+}
 
 MCSchedModel
 MCSubtargetInfo::getSchedModelForCPU(StringRef CPU) const {
   assert(ProcSchedModels && "Processor machine model not available!");
 
   unsigned NumProcs = ProcDesc.size();
 #ifndef NDEBUG
   for (size_t i = 1; i < NumProcs; i++) {

lib/MC/SubtargetFeature.cpp

if (FeatureEntry) {
     errs() << "'" << Feature
            << "' is not a recognized feature for this target"
            << " (ignoring feature)\n";
   }
 
   return Bits;
 }
 
+FeatureBitset
+SubtargetFeatures::ApplyFeatureFlag(FeatureBitset Bits, StringRef Feature,
+                                    ArrayRef<SubtargetFeatureKV> FeatureTable) {
+
+  assert(hasFlag(Feature));
+
+  // Find feature in table.
+  const SubtargetFeatureKV *FeatureEntry =
+      Find(StripFlag(Feature), FeatureTable);
+  // If there is a match
+  if (FeatureEntry) {
+    // Enable/disable feature in bits
+    if (isEnabled(Feature)) {
+      Bits |=  FeatureEntry->Value;
+
+      // For each feature that this implies, set it.
+      SetImpliedBits(Bits, FeatureEntry, FeatureTable);
+    } else {
+      Bits &= ~FeatureEntry->Value;
+
+      // For each feature that implies this, clear it.
+      ClearImpliedBits(Bits, FeatureEntry, FeatureTable);
+    }
+  } else {
+    errs() << "'" << Feature
+           << "' is not a recognized feature for this target"
+           << " (ignoring feature)\n";
+  }
+
+  return Bits;
+}
+
 
 /// getFeatureBits - Get feature bits a CPU.
 ///
 FeatureBitset
 SubtargetFeatures::getFeatureBits(StringRef CPU,
                                   ArrayRef<SubtargetFeatureKV> CPUTable,
                                   ArrayRef<SubtargetFeatureKV> FeatureTable) {
 
#endif
   }
 
   // Iterate through each feature
   for (auto &Feature : Features) {
     // Check for help
     if (Feature == "+help")
       Help(CPUTable, FeatureTable);
 
-    // Find feature in table.
-    const SubtargetFeatureKV *FeatureEntry =
-        Find(StripFlag(Feature), FeatureTable);
-    // If there is a match
-    if (FeatureEntry) {
-      // Enable/disable feature in bits
-      if (isEnabled(Feature)) {
-        Bits |=  FeatureEntry->Value;
-
-        // For each feature that this implies, set it.
-        SetImpliedBits(Bits, FeatureEntry, FeatureTable);
-      } else {
-        Bits &= ~FeatureEntry->Value;
-
-        // For each feature that implies this, clear it.
-        ClearImpliedBits(Bits, FeatureEntry, FeatureTable);
-      }
-    } else {
-      errs() << "'" << Feature
-             << "' is not a recognized feature for this target"
-             << " (ignoring feature)\n";
-    }
+    Bits = ApplyFeatureFlag(Bits, Feature, FeatureTable);
   }
 
   return Bits;
 }
 
 /// print - Print feature string.
 ///
 void SubtargetFeatures::print(raw_ostream &OS) const {

lib/Support/TargetParser.cpp

 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include <cctype>
 
 using namespace llvm;
 
 namespace {
 
-// List of canonical FPU names (use getFPUSynonym).
+// An FPU name can imply one of three levels of Neon support: none, neon, and
+// neon with crypto.
+enum NeonSupportLevel {
+  NS_None,
+  NS_Neon,
+  NS_Crypto
+};
+
+// An FPU name can restrict the FPU in one of three ways: no restriction, d16
+// (only 16 D registers), and sp-d16 (only single-precision instructions and
+// only 16 D registers).
+enum FPURestriction {
+  FR_None,
+  FR_D16,
+  FR_SP_D16
+};
+
+// List of canonical FPU names (use getFPUSynonym) and which architectural
+// features they correspond to (use getFPUFeatures).
 // FIXME: TableGen this.
 struct {
   const char * Name;
   ARM::FPUKind ID;
+  // FPUVersion corresponds directly to the FP architecture version number.
+  unsigned FPUVersion;
+  NeonSupportLevel NeonSupport;
+  FPURestriction Restriction;
 } FPUNames[] = {
-  { "invalid",              ARM::FK_INVALID },
-  { "vfp",                  ARM::FK_VFP },
-  { "vfpv2",                ARM::FK_VFPV2 },
-  { "vfpv3",                ARM::FK_VFPV3 },
-  { "vfpv3-d16",            ARM::FK_VFPV3_D16 },
-  { "vfpv4",                ARM::FK_VFPV4 },
-  { "vfpv4-d16",            ARM::FK_VFPV4_D16 },
-  { "fpv5-d16",             ARM::FK_FPV5_D16 },
-  { "fp-armv8",             ARM::FK_FP_ARMV8 },
-  { "neon",                 ARM::FK_NEON },
-  { "neon-vfpv4",           ARM::FK_NEON_VFPV4 },
-  { "neon-fp-armv8",        ARM::FK_NEON_FP_ARMV8 },
-  { "crypto-neon-fp-armv8", ARM::FK_CRYPTO_NEON_FP_ARMV8 },
-  { "softvfp",              ARM::FK_SOFTVFP }
+  { "invalid",              ARM::FK_INVALID,       0, NS_None,   FR_None},
+  { "vfp",                  ARM::FK_VFP,           2, NS_None,   FR_None},
+  { "vfpv2",                ARM::FK_VFPV2,         2, NS_None,   FR_None},
+  { "vfpv3",                ARM::FK_VFPV3,         3, NS_None,   FR_None},
+  { "vfpv3-d16",            ARM::FK_VFPV3_D16,     3, NS_None,   FR_D16},
+  { "vfpv4",                ARM::FK_VFPV4,         4, NS_None,   FR_None},
+  { "vfpv4-d16",            ARM::FK_VFPV4_D16,     4, NS_None,   FR_D16},
+  { "fpv5-d16",             ARM::FK_FPV5_D16,      5, NS_None,   FR_D16},
+  { "fp-armv8",             ARM::FK_FP_ARMV8,      5, NS_None,   FR_None},
+  { "neon",                 ARM::FK_NEON,          3, NS_Neon,   FR_None},
+  { "neon-vfpv4",           ARM::FK_NEON_VFPV4,    4, NS_Neon,   FR_None},
+  { "neon-fp-armv8",        ARM::FK_NEON_FP_ARMV8, 5, NS_Neon,   FR_None},
+  { "crypto-neon-fp-armv8", ARM::FK_CRYPTO_NEON_FP_ARMV8,
+                                                   5, NS_Crypto, FR_None},
+  { "softvfp",              ARM::FK_SOFTVFP,       0, NS_None,   FR_None},
 };
+
 // List of canonical arch names (use getArchSynonym).
 // This table also provides the build attribute fields for CPU arch
 // and Arch ID, according to the Addenda to the ARM ABI, chapters
 // 2.4 and 2.3.5.2 respectively.
 // FIXME: SubArch values were simplified to fit into the expectations
 // of the triples and are not conforming with their official names.
 // Check to see if the expectation should be changed.
 // FIXME: TableGen this.

 // ======================================================= //
 
 const char *ARMTargetParser::getFPUName(unsigned FPUKind) {
   if (FPUKind >= ARM::FK_LAST)
     return nullptr;
   return FPUNames[FPUKind].Name;
 }
 
+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.
+  switch (FPUNames[FPUKind].Restriction) {
+  case FR_SP_D16:
+    Features.push_back("+fp-only-sp");
+    Features.push_back("+d16");
+    break;
+  case FR_D16:
+    Features.push_back("-fp-only-sp");
+    Features.push_back("+d16");
+    break;
+  case FR_None:
+    Features.push_back("-fp-only-sp");
+    Features.push_back("-d16");
+    break;
+  }
+
+  // FPU version subtarget features are inclusive of lower-numbered ones, so
+  // enable the one corresponding to this version and disable all that are
+  // higher.
+  switch (FPUNames[FPUKind].FPUVersion) {
+  case 5:
+    Features.push_back("+fp-armv8");
+    break;
+  case 4:
+    Features.push_back("+vfp4");
+    Features.push_back("-fp-armv8");
+    break;
+  case 3:
+    Features.push_back("+vfp3");
+    Features.push_back("-vfp4");
+    Features.push_back("-fp-armv8");
+    break;
+  case 2:
+    Features.push_back("+vfp2");
+    Features.push_back("-vfp3");
+    Features.push_back("-vfp4");
+    Features.push_back("-fp-armv8");
+    break;
+  case 0:
+    Features.push_back("-vfp2");
+    Features.push_back("-vfp3");
+    Features.push_back("-vfp4");
+    Features.push_back("-fp-armv8");
+    break;
+  }
+
+  // crypto includes neon, so we handle this similarly to FPU version.
+  switch (FPUNames[FPUKind].NeonSupport) {
+  case NS_Crypto:
+    Features.push_back("+crypto");
+    break;
+  case NS_Neon:
+    Features.push_back("+neon");
+    Features.push_back("-crypto");
+    break;
+  case NS_None:
+    Features.push_back("-neon");
+    Features.push_back("-crypto");
+    break;
+  }
+
+  return true;
+}
+
 const char *ARMTargetParser::getArchName(unsigned ArchKind) {
   if (ArchKind >= ARM::AK_LAST)
     return nullptr;
   return ARCHNames[ArchKind].Name;
 }
 
 const char *ARMTargetParser::getCPUAttr(unsigned ArchKind) {
   if (ArchKind >= ARM::AK_LAST)

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

bool ARMAsmParser::parseDirectiveCPU(SMLoc L) {
   }
 
   STI.InitMCProcessorInfo(CPU, "");
   STI.InitCPUSchedModel(CPU);
   setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
 
   return false;
 }
-
-// FIXME: This is duplicated in getARMFPUFeatures() in
-// tools/clang/lib/Driver/Tools.cpp
-static const struct {
-  const unsigned ID;
-  const FeatureBitset Enabled;
-  const FeatureBitset Disabled;
-} FPUs[] = {
-    {/* ID */ ARM::FK_VFP, 
-     /* Enabled */ {ARM::FeatureVFP2}, 
-     /* Disabled */ {ARM::FeatureNEON}},
-    {/* ID */ ARM::FK_VFPV2, 
-     /* Enabled */ {ARM::FeatureVFP2}, 
-     /* Disabled */ {ARM::FeatureNEON}},
-    {/* ID */ ARM::FK_VFPV3, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3},  
-     /* Disabled */ {ARM::FeatureNEON, ARM::FeatureD16}},
-    {/* ID */ ARM::FK_VFPV3_D16, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureD16},
-     /* Disabled */ {ARM::FeatureNEON}},
-    {/* ID */ ARM::FK_VFPV4, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureVFP4},
-     /* Disabled */ {ARM::FeatureNEON, ARM::FeatureD16}},
-    {/* ID */ ARM::FK_VFPV4_D16, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureVFP4,
-                    ARM::FeatureD16},
-     /* Disabled */ {ARM::FeatureNEON}},
-    {/* ID */ ARM::FK_FPV5_D16, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureVFP4,
-                    ARM::FeatureFPARMv8, ARM::FeatureD16},
-     /* Disabled */ {ARM::FeatureNEON, ARM::FeatureCrypto}},
-    {/* ID */ ARM::FK_FP_ARMV8, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureVFP4,
-                    ARM::FeatureFPARMv8},
-     /* Disabled */ {ARM::FeatureNEON, ARM::FeatureCrypto, ARM::FeatureD16}},
-    {/* ID */ ARM::FK_NEON, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureNEON}, 
-     /* Disabled */ {ARM::FeatureD16}},
-    {/* ID */ ARM::FK_NEON_VFPV4, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureVFP4,
-                    ARM::FeatureNEON}, 
-     /* Disabled */ {ARM::FeatureD16}},
-    {/* ID */ ARM::FK_NEON_FP_ARMV8, 
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureVFP4,
-                    ARM::FeatureFPARMv8, ARM::FeatureNEON},
-     /* Disabled */ {ARM::FeatureCrypto, ARM::FeatureD16}},
-    {/* ID */ ARM::FK_CRYPTO_NEON_FP_ARMV8,
-     /* Enabled */ {ARM::FeatureVFP2, ARM::FeatureVFP3, ARM::FeatureVFP4,
-                    ARM::FeatureFPARMv8, ARM::FeatureNEON, 
-                    ARM::FeatureCrypto},
-     /* Disabled */ {ARM::FeatureD16}},
-    {ARM::FK_SOFTVFP, {}, {}},
-};
-
 /// parseDirectiveFPU
 ///  ::= .fpu str
 bool ARMAsmParser::parseDirectiveFPU(SMLoc L) {
   SMLoc FPUNameLoc = getTok().getLoc();
   StringRef FPU = getParser().parseStringToEndOfStatement().trim();
 
   unsigned ID = ARMTargetParser::parseFPU(FPU);
-
-  if (ID == ARM::FK_INVALID) {
+  std::vector<const char *> Features;
+  if (!ARMTargetParser::getFPUFeatures(ID, Features)) {
     Error(FPUNameLoc, "Unknown FPU name");
     return false;
   }
 
-  for (const auto &Entry : FPUs) {
-    if (Entry.ID != ID)
-      continue;
-
-    // Need to toggle features that should be on but are off and that
-    // should off but are on.
-    FeatureBitset Toggle = (Entry.Enabled & ~STI.getFeatureBits()) |
-                           (Entry.Disabled & STI.getFeatureBits());
-    setAvailableFeatures(ComputeAvailableFeatures(STI.ToggleFeature(Toggle)));
-    break;
+  for (auto Feature : Features) {
+    STI.ApplyFeatureFlag(Feature);
   }
+  setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
 
   getTargetStreamer().emitFPU(ID);
   return false;
 }
 
 /// parseDirectiveFnStart
 ///  ::= .fnstart
 bool ARMAsmParser::parseDirectiveFnStart(SMLoc L) {