Normalize the features string in SubtargetFeatures::getFeatureBits

[tyomitch]

Summary

Currently, specifying a feature string such as +fullfp16,-fp-armv8,+fp-armv8,+neon
leads to fullfp16 being disabled (transitively by -fp-armv8, and not re-enabled
by +fp-armv8).

It would be far more natural to make +fullfp16,-fp-armv8,+fp-armv8,+neon
equivalent to +fullfp16,+neon, matching the intuitive arithmetic and/or set-theory rules.

Note that this still doesn't make feature strings commutative:
e.g. +fullfp16,+fp-armv8,-fp-armv8,+neon would still disable fullfp16.

While there, this patch also changes SubtargetFeatures::ToggleFeature and
SubtargetFeatures::ApplyFeatureFlag to be static, so that MCSubtargetInfo
doesn't need to instantiate SubtargetFeatures for nothing.

[mkuper]

The changes to SubtargetFeatures::ToggleFeature and SubtargetFeatures::ApplyFeatureFlag LGTM, thanks for making them.

As to the actual patch, I'm not sure whether normalizing like this is really the desired behavior or not.
I'm not against it, as such, but I'd like to hear Eric's opinion.

[echristo]

Hi,

Couple of things:

a) could you split this out? I agree that the changes to SubtargetFeatures::ToggleFeature and SubtargetFeatures::ApplyFeatureFlag look good and should be committed, but separately since the rest should have some discussion.

b) I agree with the overall notion of this, that said, I think this might be the wrong way to achieve it. How are you getting the feature strings that you're showing here? If you've got a feature that is transitively enabled or disabled by another feature then you've got a set and not a feature and it should probably be split accordingly and not just based on command line option. I think knowing the answer to my first question will show how to proceed here though.

-eric

[tyomitch]

Thank you Michael and Eric for your comments!
I've now committed the non-controversial changes as r256823.

How are you getting the feature strings that you're showing here?

We have a sort of testing script which runs LLVM with different
combinations of enabled features, pasting together chunks of
feature strings, each such chunk enabling a different subset
of test cases.

The existing implementation of getFeatureBits lets us "cancel out"
a +feature in a later chunk by appending a -feature; but
it doesn't allow to cancel out a -feature, which is what my
patch tries to achieve.

[tyomitch]

Ping?

[tyomitch]

In D15746#326637, @tyomitch wrote:

Ping?

Ping again

[echristo]

I think canceling out may be the wrong way here.

There's a chain of thought here that changes in the middle, let's go through how I was thinking and then rethinking:

Seeing this string:

+fullfp16,-fp-armv8,+fp-armv8,+neon

and knowing that fp-armv8 is a "lower level" fp than fullfp16 I'd expect that at the end we'd have fp-armv8 and neon turned on. Similarly to how if you passed the x86 equivalent on the command line:

-msse4.2 -mno-sse2 -msse3

on the command line and get:

"target-features"="+fxsr,+mmx,+sse,+sse2,+sse3,-aes,-avx,-avx2,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512pf,-avx512vl,-f16c,-fma,-fma4,-pclmul,-sha,-sse4.1,-sse4.2,-sse4a,-ssse3,-xop,-xsave,-xsaveopt"

which turns off everything sse2 and below and then turns on eveything up to sse3.

OK, that almost makes sense, however:

As a side, but important, note: I was going to use -mno-sse3 -msse3 as part of this example. It appears that the front end currently will take that as a last one wins and ignore the -mno-sse3. What would happen in your testcase if it were -, -, +? Last one wins?

Do we want last one wins here? Do we want all pairs to be merged and the one that's not a pair gone? Before we handle earlier ones? Ultimately I think we might want to assume that canonicalization has happened before we get to this point, i.e. maybe even disallow attribute strings that turn off features that were explicitly turned on earlier (not implicitly mind).

At any rate, thoughts?

:)

-eric

[tyomitch]

Thank you Eric for your detailed comments!

Do I understand correctly that this is your main concern:

OK, that almost makes sense, however:

As a side, but important, note: I was going to use -mno-sse3 -msse3 as part of this example. It appears that the front end currently will take that as a last one wins and ignore the -mno-sse3. What would happen in your testcase if it were -, -, +? Last one wins?

Do we want last one wins here? Do we want all pairs to be merged and the one that's not a pair gone? Before we handle earlier ones? Ultimately I think we might want to assume that canonicalization has happened before we get to this point, i.e. maybe even disallow attribute strings that turn off features that were explicitly turned on earlier (not implicitly mind).

That's correct, my patch keeps the existing "last one wins" handling: "+sse3,+sse3,-sse3" would (still) be treated identically to "-sse3".
The only case where "last one wins" doesn't currently apply, is when a super-feature is disabled by disabling a sub-feature, but isn't reenabled afterwards.
In other words, "+fullfp16,-fp-armv8,+fp-armv8" isn't currently equivalent to "+fullfp16,+fp-armv8".
My patch would fix this inconsistency, so that "last one wins" would apply universally.

[tyomitch]

In D15746#342099, @tyomitch wrote:

My patch would fix this inconsistency, so that "last one wins" would apply universally.

Ping?

[tyomitch]

In D15746#348456, @tyomitch wrote:

In D15746#342099, @tyomitch wrote:

My patch would fix this inconsistency, so that "last one wins" would apply universally.

Ping?

Ping again

[tyomitch]

In D15746#359578, @tyomitch wrote:

In D15746#348456, @tyomitch wrote:

In D15746#342099, @tyomitch wrote:

My patch would fix this inconsistency, so that "last one wins" would apply universally.

Ping?

Ping again

[tyomitch]

Ping again

[echristo]

In D15746#342099, @tyomitch wrote:

Thank you Eric for your detailed comments!

Do I understand correctly that this is your main concern:

OK, that almost makes sense, however:

As a side, but important, note: I was going to use -mno-sse3 -msse3 as part of this example. It appears that the front end currently will take that as a last one wins and ignore the -mno-sse3. What would happen in your testcase if it were -, -, +? Last one wins?

Do we want last one wins here? Do we want all pairs to be merged and the one that's not a pair gone? Before we handle earlier ones? Ultimately I think we might want to assume that canonicalization has happened before we get to this point, i.e. maybe even disallow attribute strings that turn off features that were explicitly turned on earlier (not implicitly mind).

That's correct, my patch keeps the existing "last one wins" handling: "+sse3,+sse3,-sse3" would (still) be treated identically to "-sse3".
The only case where "last one wins" doesn't currently apply, is when a super-feature is disabled by disabling a sub-feature, but isn't reenabled afterwards.
In other words, "+fullfp16,-fp-armv8,+fp-armv8" isn't currently equivalent to "+fullfp16,+fp-armv8".
My patch would fix this inconsistency, so that "last one wins" would apply universally.

I'm still uncertain we want to do this in the backend. I seem to recall you wanting to do this for a reason, but searching my email can't find it. Why can't we rely/assert/verify that the list of features in the list is complete and non-contradictory rather than coping with "garbage" in the backend. This seems like something the front-end, or whatever is generating IR, should handle.

[tyomitch]

In D15746#381104, @echristo wrote:

I'm still uncertain we want to do this in the backend. I seem to recall you wanting to do this for a reason, but searching my email can't find it.

In D15746#319426, @tyomitch wrote:

How are you getting the feature strings that you're showing here?

We have a sort of testing script which runs LLVM with different
combinations of enabled features, pasting together chunks of
feature strings, each such chunk enabling a different subset
of test cases.

The existing implementation of getFeatureBits lets us "cancel out"
a +feature in a later chunk by appending a -feature; but
it doesn't allow to cancel out a -feature, which is what my
patch tries to achieve.

In D15746#381104, @echristo wrote:

Why can't we rely/assert/verify that the list of features in the list is complete and non-contradictory rather than coping with "garbage" in the backend. This seems like something the front-end, or whatever is generating IR, should handle.

Pre-parsing the feature string to "sanitize" it is certainly a possibility; my preference for doing this in SubtargetFeatures::getFeatureBits was to reuse the existing feature string parser, and avoid code duplication.

[echristo]

I mean basically not allowing the use that you're trying for there. Any duplications or changes aren't going to matter.

Also, FWIW, any subtarget feature that isn't isolated really isn't good. I.e. you should have to explicitly turn on both fp-armv8 and fullfp16 if you want both enabled.

-eric

include/llvm/MC/SubtargetFeature.h

public:
   explicit SubtargetFeatures(StringRef Initial = "");
 
   /// Features string accessors.
   std::string getString() const;
 
   /// 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,
+  /// ToggleFeature - Toggle a feature and update the feature bits.
+  static void 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);
+  /// Apply the feature flag and update the feature bits.
+  static void ApplyFeatureFlag(FeatureBitset &Bits, StringRef Feature,
 
   /// 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;

lib/MC/MCSubtargetInfo.cpp

 FeatureBitset MCSubtargetInfo::ToggleFeature(const FeatureBitset &FB) {
   FeatureBits ^= FB;
   return FeatureBits;
 }
 
 /// 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);
+  SubtargetFeatures::ToggleFeature(FeatureBits, FS, ProcFeatures);
   return FeatureBits;
 }
 
 FeatureBitset MCSubtargetInfo::ApplyFeatureFlag(StringRef FS) {
-  SubtargetFeatures Features;
-  FeatureBits = Features.ApplyFeatureFlag(FeatureBits, FS, ProcFeatures);
+  SubtargetFeatures::ApplyFeatureFlag(FeatureBits, FS, ProcFeatures);
   return FeatureBits;
 }
 
 const MCSchedModel &MCSubtargetInfo::getSchedModelForCPU(StringRef CPU) const {
   assert(ProcSchedModels && "Processor machine model not available!");
 
   size_t NumProcs = ProcDesc.size();
   assert(std::is_sorted(ProcSchedModels, ProcSchedModels+NumProcs,

lib/MC/SubtargetFeature.cpp

if ((FE.Implies & FeatureEntry->Value).any()) {
       Bits &= ~FE.Value;
       ClearImpliedBits(Bits, &FE, FeatureTable);
     }
   }
 }
 
 /// ToggleFeature - Toggle a feature and returns the newly updated feature
 /// bits.
-FeatureBitset
-SubtargetFeatures::ToggleFeature(FeatureBitset Bits, StringRef Feature,
+void
+SubtargetFeatures::ToggleFeature(FeatureBitset &Bits, StringRef Feature,
                                  ArrayRef<SubtargetFeatureKV> FeatureTable) {
 
   // Find feature in table.
   const SubtargetFeatureKV *FeatureEntry =
       Find(StripFlag(Feature), FeatureTable);
   // If there is a match
   if (FeatureEntry) {
     if ((Bits & FeatureEntry->Value) == FeatureEntry->Value) {
       Bits &= ~FeatureEntry->Value;
       // For each feature that implies this, clear it.
       ClearImpliedBits(Bits, FeatureEntry, FeatureTable);
     } else {
       Bits |=  FeatureEntry->Value;
 
       // For each feature that this implies, set it.
       SetImpliedBits(Bits, FeatureEntry, FeatureTable);
     }
   } else {
     errs() << "'" << Feature
            << "' is not a recognized feature for this target"
            << " (ignoring feature)\n";
   }
-
-  return Bits;
 }
 
-FeatureBitset
-SubtargetFeatures::ApplyFeatureFlag(FeatureBitset Bits, StringRef Feature,
+void 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;
+      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,
if (CPUEntry) {
       }
     } else {
       errs() << "'" << CPU
              << "' is not a recognized processor for this target"
              << " (ignoring processor)\n";
     }
   }
 
+  // Cancel out the disabled features which are explicitly re-enabled later on.
+  FeatureBitset ExplicitlyEnabled;
+  for (auto Feature  = Features.rbegin();
+            Feature != Features.rend();
+          ++Feature) {
+    const SubtargetFeatureKV *FeatureEntry =
+        Find(StripFlag(*Feature), FeatureTable);
+    if (FeatureEntry) {
+      if ((ExplicitlyEnabled & FeatureEntry->Value).any())
+        Features.erase(std::next(Feature).base());
+      else if (isEnabled(*Feature))
+        ExplicitlyEnabled |= FeatureEntry->Value;
+    }
+  }
+
   // Iterate through each feature
   for (auto &Feature : Features) {
     // Check for help
     if (Feature == "+help")
       Help(CPUTable, FeatureTable);
 
-    Bits = ApplyFeatureFlag(Bits, Feature, FeatureTable);
+    ApplyFeatureFlag(Bits, Feature, FeatureTable);
   }
 
   return Bits;
 }
 
 /// print - Print feature string.
 ///
 void SubtargetFeatures::print(raw_ostream &OS) const {

test/MC/ARM/fullfp16-neon.s

-@ RUN: llvm-mc -triple armv8a-none-eabi -mattr=+fullfp16,+neon -show-encoding < %s | FileCheck %s --check-prefix=ARM
+@ RUN: llvm-mc -triple armv8a-none-eabi -mattr=+fullfp16,-fp-armv8,+fp-armv8,+neon -show-encoding < %s | FileCheck %s --check-prefix=ARM
 @ RUN: llvm-mc -triple thumbv8a-none-eabi -mattr=+fullfp16,+neon -show-encoding < %s | FileCheck %s --check-prefix=THUMB
 
   vadd.f16 d0, d1, d2
   vadd.f16 q0, q1, q2
 @ ARM:   vadd.f16        d0, d1, d2      @ encoding: [0x02,0x0d,0x11,0xf2]
 @ ARM:   vadd.f16        q0, q1, q2      @ encoding: [0x44,0x0d,0x12,0xf2]
 @ THUMB: vadd.f16        d0, d1, d2      @ encoding: [0x11,0xef,0x02,0x0d]
 @ THUMB: vadd.f16        q0, q1, q2      @ encoding: [0x12,0xef,0x44,0x0d]