[llvm] Fix MCSubtargetInfo::checkFeatures to handle unknown features correctly

[dblaikie]

llvm/lib/MC/MCSubtargetInfo.cpp

81–87

avoid else-after-return (per LLVM coding conventions)

And it might even be worth inverting the previous condition (so it's if (!FeatureEntry) { error stuff return false; } non-error stuff here)

But also: we can't/don't usually print errors from arbitrary partts of LLVM - it's not use-LLVM-as-a-library friendly (the library client can't stop these messages from printing, render them in a different way/to a different mechanism (a log, a GUI, etc))

Are there other error handling conventions in use in nearby code you can find some inspiration from?

[craig.topper]

llvm/lib/MC/MCSubtargetInfo.cpp

313

Why do we need an Empty set to compare to? Can't we just ask if no bits are set in Set and All? Set.none() && All.none() should work?

[RamNalamothu]

Address review comments.

[RamNalamothu]

Thanks for the comments @dblaikie @craig.topper.

llvm/lib/MC/MCSubtargetInfo.cpp

81–87

avoid else-after-return (per LLVM coding conventions)

And it might even be worth inverting the previous condition (so it's if (!FeatureEntry) { error stuff return false; } non-error stuff here)

Done, thank you.

But also: we can't/don't usually print errors from arbitrary partts of LLVM - it's not use-LLVM-as-a-library friendly (the library client can't stop these messages from printing, render them in a different way/to a different mechanism (a log, a GUI, etc))

Are there other error handling conventions in use in nearby code you can find some inspiration from?

Since the entire MCSubtargetInfo.cpp is using this same error printing mechanism and I can't find a quick fix, I think that change can be deferred to another patch?

[RamNalamothu]

Ping.

[RamNalamothu]

Ping.

[dblaikie]

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

[RamNalamothu]

In D143645#4148087, @dblaikie wrote:

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

Without this change, few lit tests will fail with the updated changeset for D140478. Would that be sufficient to showcase that this change is needed?

[dblaikie]

In D143645#4150034, @RamNalamothu wrote:

In D143645#4148087, @dblaikie wrote:

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

Without this change, few lit tests will fail with the updated changeset for D140478. Would that be sufficient to showcase that this change is needed?

Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?

[craig.topper]

In D143645#4151078, @dblaikie wrote:

In D143645#4150034, @RamNalamothu wrote:

In D143645#4148087, @dblaikie wrote:

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

Without this change, few lit tests will fail with the updated changeset for D140478. Would that be sufficient to showcase that this change is needed?

Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?

That's what it looks like to me. I also think that some virtual methods a target can override would be a better way to solve this.

In D143645#4151078, @dblaikie wrote:

In D143645#4150034, @RamNalamothu wrote:

In D143645#4148087, @dblaikie wrote:

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

Without this change, few lit tests will fail with the updated changeset for D140478. Would that be sufficient to showcase that this change is needed?

Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?

[RamNalamothu]

In D143645#4151098, @craig.topper wrote:

In D143645#4151078, @dblaikie wrote:

In D143645#4150034, @RamNalamothu wrote:

In D143645#4148087, @dblaikie wrote:

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

Without this change, few lit tests will fail with the updated changeset for D140478. Would that be sufficient to showcase that this change is needed?

Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?

That's what it looks like to me. I also think that some virtual methods a target can override would be a better way to solve this.

In D143645#4151078, @dblaikie wrote:

In D143645#4150034, @RamNalamothu wrote:

In D143645#4148087, @dblaikie wrote:

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

Without this change, few lit tests will fail with the updated changeset for D140478. Would that be sufficient to showcase that this change is needed?

Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?

In D143645#4151098, @craig.topper wrote:

In D143645#4151078, @dblaikie wrote:

In D143645#4150034, @RamNalamothu wrote:

In D143645#4148087, @dblaikie wrote:

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

Without this change, few lit tests will fail with the updated changeset for D140478. Would that be sufficient to showcase that this change is needed?

Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?

That's what it looks like to me. I also think that some virtual methods a target can override would be a better way to solve this.

In D143645#4151078, @dblaikie wrote:

In D143645#4150034, @RamNalamothu wrote:

In D143645#4148087, @dblaikie wrote:

Is this new behavior visible through some existing non-test callers of MCSubtargetInfo::checkFeatures? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?

Without this change, few lit tests will fail with the updated changeset for D140478. Would that be sufficient to showcase that this change is needed?

Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?

Thanks for the comments.

Even if we find some other way to address the needs of D140478, I think the incorrect behavior of MCSubtargetInfo::checkFeatures needs to be fixed.

Is having no existing code referring to the use case scenario being fixed a good reason we leave the incorrect behavior in the code base as is?

[dblaikie]

Thanks for the comments.

Even if we find some other way to address the needs of D140478, I think the incorrect behavior of MCSubtargetInfo::checkFeatures needs to be fixed.

Is having no existing code referring to the use case scenario being fixed a good reason we leave the incorrect behavior in the code base as is?

I'm not sure this is incorrect behavior, though - that this function doesn't handle unknown attributes seems like it could be a valid design. If there's other ways to think about this that demonstrate that the design is not good/incorrect for existing uses, perhaps, maybe that motivates this patch regardless of D140478.

[arsenm]

It seems like the only use of checkFeatures is for implementing hwmodes?

llvm/unittests/CodeGen/TargetOptionsTest.cpp

71

It's a gtest-ism that these should be swapped

llvm/lib/MC/MCSubtargetInfo.cpp

void ClearImpliedBits(FeatureBitset &Bits, unsigned Value,
   for (const SubtargetFeatureKV &FE : FeatureTable) {
     if (FE.Implies.getAsBitset().test(Value)) {
       Bits.reset(FE.Value);
       ClearImpliedBits(Bits, FE.Value, FeatureTable);
     }
   }
 }
 
-static void ApplyFeatureFlag(FeatureBitset &Bits, StringRef Feature,
+static bool ApplyFeatureFlag(FeatureBitset &Bits, StringRef Feature,
                              ArrayRef<SubtargetFeatureKV> FeatureTable) {
   assert(SubtargetFeatures::hasFlag(Feature) &&
          "Feature flags should start with '+' or '-'");
 
   // Find feature in table.
   const SubtargetFeatureKV *FeatureEntry =
       Find(SubtargetFeatures::StripFlag(Feature), FeatureTable);
   // If there is a match
   if (FeatureEntry) {
     // Enable/disable feature in bits
     if (SubtargetFeatures::isEnabled(Feature)) {
       Bits.set(FeatureEntry->Value);
 
       // For each feature that this implies, set it.
       SetImpliedBits(Bits, FeatureEntry->Implies.getAsBitset(), FeatureTable);
     } else {
       Bits.reset(FeatureEntry->Value);
 
       // For each feature that implies this, clear it.
       ClearImpliedBits(Bits, FeatureEntry->Value, FeatureTable);
     }
+    return true;
   } else {
     errs() << "'" << Feature << "' is not a recognized feature for this target"
            << " (ignoring feature)\n";
+    return false;
   }
+  return false;
 }
 
 /// Return the length of the longest entry in the table.
 template <typename T>
 static size_t getLongestEntryLength(ArrayRef<T> Table) {
   size_t MaxLen = 0;
   for (auto &I : Table)
     MaxLen = std::max(MaxLen, std::strlen(I.Key));

 
 FeatureBitset MCSubtargetInfo::ApplyFeatureFlag(StringRef FS) {
   ::ApplyFeatureFlag(FeatureBits, FS, ProcFeatures);
   return FeatureBits;
 }
 
 bool MCSubtargetInfo::checkFeatures(StringRef FS) const {
   SubtargetFeatures T(FS);
-  FeatureBitset Set, All;
+  FeatureBitset Set, All, Empty;
   for (std::string F : T.getFeatures()) {
-    ::ApplyFeatureFlag(Set, F, ProcFeatures);
+    if (!::ApplyFeatureFlag(Set, F, ProcFeatures))
+      continue;
     if (F[0] == '-')
       F[0] = '+';
-    ::ApplyFeatureFlag(All, F, ProcFeatures);
+    (void)::ApplyFeatureFlag(All, F, ProcFeatures);
   }
+
+  // All input features are unknown features.
+  if (Set == Empty && All == Empty)
+    return false;
+
   return (FeatureBits & All) == Set;
 }
 
 const MCSchedModel &MCSubtargetInfo::getSchedModelForCPU(StringRef CPU) const {
   assert(llvm::is_sorted(ProcDesc) &&
          "Processor machine model table is not sorted");
 
   // Find entry

llvm/unittests/CodeGen/TargetOptionsTest.cpp

 #include "llvm/Target/TargetOptions.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Target/TargetMachine.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace llvm {
std::unique_ptr<TargetMachine> createTargetMachine(bool EnableIPRA) {
   std::string Error;
   const Target *T = TargetRegistry::lookupTarget("", TargetTriple, Error);
   if (!T)
     return nullptr;
 
   TargetOptions Options;
   Options.EnableIPRA = EnableIPRA;
   return std::unique_ptr<TargetMachine>(
-      T->createTargetMachine("X86", "", "", Options, std::nullopt, std::nullopt,
-                             CodeGenOpt::Aggressive));
+      T->createTargetMachine("X86", "", "+avx", Options, std::nullopt,
+                             std::nullopt, CodeGenOpt::Aggressive));
 }
 
 typedef std::function<void(bool)> TargetOptionsTest;
 
 static void targetOptionsTest(bool EnableIPRA) {
   std::unique_ptr<TargetMachine> TM = createTargetMachine(EnableIPRA);
   // This test is designed for the X86 backend; stop if it is not available.
   if (!TM)
     GTEST_SKIP();
   legacy::PassManager PM;
   LLVMTargetMachine *LLVMTM = static_cast<LLVMTargetMachine *>(TM.get());
 
   TargetPassConfig *TPC = LLVMTM->createPassConfig(PM);
   (void)TPC;
 
   ASSERT_TRUE(TM->Options.EnableIPRA == EnableIPRA);
 
   delete TPC;
 }
 
+static void targetFeaturesTest(StringRef F) {
+  std::unique_ptr<TargetMachine> TM = createTargetMachine(false);
+  // This test is designed for the X86 backend; stop if it is not available.
+  if (!TM)
+    GTEST_SKIP();
+
+  ASSERT_NE(TM->getMCSubtargetInfo(), nullptr);
+
+  ASSERT_FALSE(TM->getMCSubtargetInfo()->checkFeatures(F));
+}
+
 } // End of anonymous namespace.
 
 TEST(TargetOptionsTest, IPRASetToOff) {
   targetOptionsTest(false);
 }
 
 TEST(TargetOptionsTest, IPRASetToOn) {
   targetOptionsTest(true);
 }
 
+TEST(TargetOptionsTest, KnownFeature) { targetFeaturesTest("-avx"); }
+
+TEST(TargetOptionsTest, UnknownFeature) { targetFeaturesTest("+relax"); }
+
 int main(int argc, char **argv) {
   ::testing::InitGoogleTest(&argc, argv);
   initLLVM();
   return RUN_ALL_TESTS();
 }