[gicombiner] Allow disable-rule option to disable all-except-...

[dsanders]

Summary

Adds two features to the generated rule disable option:

• '*' - Disable all rules
• '!<foo>' - Re-enable rule(s)
  • '!foo' - Enable rule named 'foo'
  • '!5' - Enable rule five
  • '!4-9' - Enable rule four to nine
  • '!foo-bar' - Enable rules from 'foo' to (and including) 'bar'

(the '!' is available to the generated disable option but is not part of the underlying and determines whether to call setRuleDisabled() or setRuleEnabled())

This is intended to support unit testing of combine rules so
that you can do:

GeneratedCfg.setRuleDisabled("*")
GeneratedCfg.setRuleEnabled("foo")

to ensure only a specific rule is in effect. The rule is still
required to be included in a combiner though

[arsenm]

It's not clear to me what "'!foo-bar' - Enable rules from 'foo' to (and including) 'bar'" means. These have some kind of ordering?

[dsanders]

In D81889#2094543, @arsenm wrote:

It's not clear to me what "'!foo-bar' - Enable rules from 'foo' to (and including) 'bar'" means. These have some kind of ordering?

Yes, it's the order that they're listed in the Combiner declaration (after flattening into a list) so it's a depth-first walk of the original list. It's mostly supported because there's no reason to special case it such that rule names don't work in the range syntax.

[paquette]

The point here is to allow people to only enable specific rules, right?

Maybe an interface like this would be a bit simpler?

--my_combiner-only-enable-rules="rule1,rule2,rule3,..."

[aditya_nandakumar]

In D81889#2096115, @paquette wrote:

The point here is to allow people to only enable specific rules, right?

Maybe an interface like this would be a bit simpler?

--my_combiner-only-enable-rules="rule1,rule2,rule3,..."

+1.
For starters we should explicitly include the combines we want (or maybe the group) similar to what @paquette suggested.

[dsanders]

In D81889#2096115, @paquette wrote:

The point here is to allow people to only enable specific rules, right?

Yes, the point of this change is to support that case. The existing option was to support debugging and bisecting by disabling rules until to see if a miscompile gets resolved and using that to pin the miscompile to a particular rule.

Maybe an interface like this would be a bit simpler?

--my_combiner-only-enable-rules="rule1,rule2,rule3,..."

The reason I didn't add a second option is because you'd have to define the interaction between them:
--my_combiner-only-enable-rules=rule1,rule2,rule3 --my_combiner-disable-rule=rule2 sounds like rule 1 and 3 should be the only ones enabled, but then
--my_combiner-disable-rule=rule2 --my_combiner-only-enable-rules=rule1,rule2,rule3 sounds like it's rule1, 2, and 3 enabled but that's not implementable with the CommandLine library AFAIK. You'd actually get rule1 and 3

In general I'd prefer not to introduce conflicting options, I'll take another look at CommandLine and see if there's anything we can use there.

[aditya_nandakumar]

To enable rules {x,y}, you'd need to disable the universe, and specify rules you want to enable with !, all in a command called disable-rule*. This sounds inverse of what's necessary and is far from ergonomic.
I see this as similar to if opt/llc run-pass invocation was done by disable universe of passes, and enable some passes with !.
Additionally for the most common and immediately useful case of writing tests, I don't expect any need to both enable and disable rules in the same invocation. IOW, for testing purposes, only use the enable opt, and for bisecting and other tools (for future), use the disable opt. Perhaps we should rename the opt to make it explicit that it's used for testing only and perhaps even we assert that both are not used simultaneously (or make it abundantly clear in the docs).
TLDR; I'm suggesting we optimize the interface for the most common and immediately useful case of testing, and worry about enabling and disabling simultaneously later on?

[dsanders]

Add only-enable-rule and make it interact with disable-rule correctly.

[aditya_nandakumar]

This looks much better. Looks good to me.

llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-copy-prop-disabled.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -run-pass=aarch64-prelegalizer-combiner -global-isel -verify-machineinstrs %s -o - \
 # RUN:                                                                | FileCheck --check-prefix=ENABLED %s
 # RUN: llc -run-pass=aarch64-prelegalizer-combiner -global-isel -verify-machineinstrs %s -o - \
 # RUN:     --aarch64prelegalizercombinerhelper-disable-rule=copy_prop | FileCheck --check-prefix=DISABLED %s
+# RUN: llc -run-pass=aarch64-prelegalizer-combiner -global-isel -verify-machineinstrs %s -o - \
+# RUN:     --aarch64prelegalizercombinerhelper-disable-rule="*" | FileCheck --check-prefix=DISABLED %s
+# RUN: llc -run-pass=aarch64-prelegalizer-combiner -global-isel -verify-machineinstrs %s -o - \
+# RUN:     --aarch64prelegalizercombinerhelper-disable-rule="*,!copy_prop" \
+# RUN:    | FileCheck --check-prefix=ENABLED %s
+# RUN: llc -run-pass=aarch64-prelegalizer-combiner -global-isel -verify-machineinstrs %s -o - \
+# RUN:     --aarch64prelegalizercombinerhelper-only-enable-rule="copy_prop" \
+# RUN:    | FileCheck --check-prefix=ENABLED %s
 
 # REQUIRES: asserts
 
 --- |
   target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
   target triple = "aarch64--"
   define void @test_copy(i8* %addr) {
   entry:

llvm/utils/TableGen/GICombinerEmitter.cpp

void GICombinerEmitter::run(raw_ostream &OS) {
 
   OS << "#ifdef " << Name.upper() << "_GENCOMBINERHELPER_H\n"
      << "class " << getClassName() << "RuleConfig {\n"
      << "  SparseBitVector<> DisabledRules;\n"
      << "\n"
      << "public:\n"
      << "  bool parseCommandLineOption();\n"
      << "  bool isRuleDisabled(unsigned ID) const;\n"
+     << "  bool setRuleEnabled(StringRef RuleIdentifier);\n"
      << "  bool setRuleDisabled(StringRef RuleIdentifier);\n"
      << "\n"
      << "};\n"
      << "\n"
      << "class " << getClassName();
   StringRef StateClass = Combiner->getValueAsString("StateClass");
   if (!StateClass.empty())
     OS << " : public " << StateClass;
OS << "RuleConfig(&RuleConfig) {}\n"
      << "    MachineInstr &MI,\n"
      << "    MachineIRBuilder &B";
   emitAdditionalHelperMethodArguments(OS, Combiner);
   OS << ") const;\n";
   OS << "};\n\n";
 
   emitNameMatcher(OS);
 
-  OS << "bool " << getClassName()
-     << "RuleConfig::setRuleDisabled(StringRef RuleIdentifier) {\n"
+  OS << "static Optional<std::pair<uint64_t, uint64_t>> "
+        "getRuleRangeForIdentifier(StringRef RuleIdentifier) {\n"
      << "  std::pair<StringRef, StringRef> RangePair = "
         "RuleIdentifier.split('-');\n"
      << "  if (!RangePair.second.empty()) {\n"
-     << "    const auto First = getRuleIdxForIdentifier(RangePair.first);\n"
-     << "    const auto Last = getRuleIdxForIdentifier(RangePair.second);\n"
+     << "    const auto First = "
+        "getRuleIdxForIdentifier(RangePair.first);\n"
+     << "    const auto Last = "
+        "getRuleIdxForIdentifier(RangePair.second);\n"
      << "    if (!First.hasValue() || !Last.hasValue())\n"
-     << "      return false;\n"
+     << "      return None;\n"
      << "    if (First >= Last)\n"
-     << "      report_fatal_error(\"Beginning of range should be before end of "
-        "range\");\n"
-     << "    for (auto I = First.getValue(); I < Last.getValue(); ++I)\n"
-     << "      DisabledRules.set(I);\n"
-     << "    return true;\n"
+     << "      report_fatal_error(\"Beginning of range should be before "
+        "end of range\");\n"
+     << "    return {{ *First, *Last + 1 }};\n"
+     << "  } else if (RangePair.first == \"*\") {\n"
+     << "    return {{ 0, " << Rules.size() << " }};\n"
      << "  } else {\n"
      << "    const auto I = getRuleIdxForIdentifier(RangePair.first);\n"
      << "    if (!I.hasValue())\n"
-     << "      return false;\n"
-     << "    DisabledRules.set(I.getValue());\n"
-     << "    return true;\n"
+     << "      return None;\n"
+     << "    return {{*I, *I + 1}};\n"
      << "  }\n"
-     << "  return false;\n"
-     << "}\n";
+     << "  return None;\n"
+     << "}\n\n";
+
+  for (bool Enabled : {true, false}) {
+    OS << "bool " << getClassName() << "RuleConfig::setRule"
+       << (Enabled ? "Enabled" : "Disabled") << "(StringRef RuleIdentifier) {\n"
+       << "  auto MaybeRange = getRuleRangeForIdentifier(RuleIdentifier);\n"
+       << "  if(!MaybeRange.hasValue())\n"
+       << "    return false;\n"
+       << "  for (auto I = MaybeRange->first; I < MaybeRange->second; ++I)\n"
+       << "    DisabledRules." << (Enabled ? "reset" : "set") << "(I);\n"
+       << "  return true;\n"
+       << "}\n\n";
+  }
 
   OS << "bool " << getClassName()
      << "RuleConfig::isRuleDisabled(unsigned RuleID) const {\n"
      << "  return DisabledRules.test(RuleID);\n"
      << "}\n";
   OS << "#endif // ifdef " << Name.upper() << "_GENCOMBINERHELPER_H\n\n";
 
   OS << "#ifdef " << Name.upper() << "_GENCOMBINERHELPER_CPP\n"
      << "\n"
-     << "cl::list<std::string> " << Name << "Option(\n"
+     << "std::vector<std::string> " << Name << "Option;\n"
+     << "cl::list<std::string> " << Name << "DisableOption(\n"
      << "    \"" << Name.lower() << "-disable-rule\",\n"
      << "    cl::desc(\"Disable one or more combiner rules temporarily in "
      << "the " << Name << " pass\"),\n"
      << "    cl::CommaSeparated,\n"
      << "    cl::Hidden,\n"
-     << "    cl::cat(GICombinerOptionCategory));\n"
+     << "    cl::cat(GICombinerOptionCategory),\n"
+     << "    cl::callback([](const std::string &Str) {\n"
+     << "      " << Name << "Option.push_back(Str);\n"
+     << "    }));\n"
+     << "cl::list<std::string> " << Name << "OnlyEnableOption(\n"
+     << "    \"" << Name.lower() << "-only-enable-rule\",\n"
+     << "    cl::desc(\"Disable all rules in the " << Name
+     << " pass then re-enable the specified ones\"),\n"
+     << "    cl::Hidden,\n"
+     << "    cl::cat(GICombinerOptionCategory),\n"
+     << "    cl::callback([](const std::string &CommaSeparatedArg) {\n"
+     << "      StringRef Str = CommaSeparatedArg;\n"
+     << "      " << Name << "Option.push_back(\"*\");\n"
+     << "      do {\n"
+     << "        auto X = Str.split(\",\");\n"
+     << "        " << Name << "Option.push_back((\"!\" + X.first).str());\n"
+     << "        Str = X.second;\n"
+     << "      } while (!Str.empty());\n"
+     << "    }));\n"
      << "\n"
      << "bool " << getClassName() << "RuleConfig::parseCommandLineOption() {\n"
-     << "  for (const auto &Identifier : " << Name << "Option)\n"
-     << "    if (!setRuleDisabled(Identifier))\n"
+     << "  for (StringRef Identifier : " << Name << "Option) {\n"
+     << "    bool Enabled = Identifier.consume_front(\"!\");\n"
+     << "    if (Enabled && !setRuleEnabled(Identifier))\n"
      << "      return false;\n"
+     << "    if (!Enabled && !setRuleDisabled(Identifier))\n"
+     << "      return false;\n"
+     << "  }\n"
      << "  return true;\n"
      << "}\n\n";
 
   OS << "bool " << getClassName() << "::tryCombineAll(\n"
      << "    GISelChangeObserver &Observer,\n"
      << "    MachineInstr &MI,\n"
      << "    MachineIRBuilder &B";
   emitAdditionalHelperMethodArguments(OS, Combiner);