[TableGen] Allow emitter callbacks to use `const RecordKeeper &` #104716

jurahul · 2024-08-18T17:44:32Z

Refactor TableGen backend options to allow specifying a callback
function that takes either a const reference or a non-const reference
to RecordKeeper. This will enable gradual migration of code to use
const references and pointers to RecordKeeper and Record in the
TableGen backends.
Refactor handling of the callback command line options. Move variable
for the command line option from the header to the CPP file, and add a
function ApplyCallback to apply the selected callback.
Change callbacks in TableGen.cpp to take const reference. They use the
Opt class to register their callbacks.
Change IntrinsicEmitter to use the OptClass to define its callbacks.
It already uses a const reference in the implementation.

llvm/utils/TableGen/TableGen.cpp

llvm/include/llvm/TableGen/TableGenBackend.h

llvm/lib/TableGen/TableGenBackend.cpp

llvmbot · 2024-08-22T02:26:03Z

@llvm/pr-subscribers-llvm-adt

Author: Rahul Joshi (jurahul)

Changes

Refactor TableGen backend options to allow specifying a callback
function that takes a const reference to RecordKeeper. This
will enable gradual migration of code to use const references
and pointers to RecordKeeper and Record in the TableGen
backend. Add support for both forms of the callback to both
Opt and OptClass.
Refactor handling of the callback command line options. Move
variable for the command line option from the header to the
CPP file, and add a function ApplyCallback to apply the
selected callback.
Change callbacks in TableGen.cpp to take const reference. They
use the Opt class to define their callbacks.
Change IntrinsicEmitter to use the OptClass to define its
callbacks. It already uses a const reference.
Change global variables in TableGen.cpp to be static instead of
being in an anonymous namespace per LLVM coding standards.

Full diff: https://github.com/llvm/llvm-project/pull/104716.diff

8 Files Affected:

(modified) llvm/include/llvm/ADT/STLFunctionalExtras.h (+4)
(modified) llvm/include/llvm/TableGen/TableGenBackend.h (+16-16)
(modified) llvm/lib/TableGen/Main.cpp (+4-7)
(modified) llvm/lib/TableGen/TableGenBackend.cpp (+49-6)
(modified) llvm/unittests/ADT/FunctionRefTest.cpp (+8)
(modified) llvm/utils/TableGen/DisassemblerEmitter.cpp (+1)
(modified) llvm/utils/TableGen/IntrinsicEmitter.cpp (+12-12)
(modified) llvm/utils/TableGen/TableGen.cpp (+4-4)

diff --git a/llvm/include/llvm/ADT/STLFunctionalExtras.h b/llvm/include/llvm/ADT/STLFunctionalExtras.h
index dd7fc6dc748645..6f172504b3c167 100644
--- a/llvm/include/llvm/ADT/STLFunctionalExtras.h
+++ b/llvm/include/llvm/ADT/STLFunctionalExtras.h
@@ -69,6 +69,10 @@ class function_ref<Ret(Params...)> {
   }
 
   explicit operator bool() const { return callback; }
+
+  bool operator==(const function_ref<Ret(Params...)> &Other) const {
+    return callable == Other.callable;
+  }
 };
 
 } // end namespace llvm
diff --git a/llvm/include/llvm/TableGen/TableGenBackend.h b/llvm/include/llvm/TableGen/TableGenBackend.h
index 9c5a785f45a403..80134179850b0e 100644
--- a/llvm/include/llvm/TableGen/TableGenBackend.h
+++ b/llvm/include/llvm/TableGen/TableGenBackend.h
@@ -13,9 +13,8 @@
 #ifndef LLVM_TABLEGEN_TABLEGENBACKEND_H
 #define LLVM_TABLEGEN_TABLEGENBACKEND_H
 
+#include "llvm/ADT/STLFunctionalExtras.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/CommandLine.h"
-#include "llvm/Support/ManagedStatic.h"
 #include "llvm/TableGen/Record.h"
 
 namespace llvm {
@@ -24,22 +23,19 @@ class RecordKeeper;
 class raw_ostream;
 
 namespace TableGen::Emitter {
-using FnT = void (*)(RecordKeeper &Records, raw_ostream &OS);
-
-struct OptCreatorT {
-  static void *call();
-};
-
-extern ManagedStatic<cl::opt<FnT>, OptCreatorT> Action;
+// Supports const and non-const forms of callback functions.
+using FnT = function_ref<void(RecordKeeper &Records, raw_ostream &OS)>;
 
+/// Creating an `Opt` object registers the command line option \p Name with
+/// TableGen backend and associates the callback \p CB with that option. If
+/// \p ByDefault is true, then that callback is applied by default if no
+/// command line option was specified.
 struct Opt {
-  Opt(StringRef Name, FnT CB, StringRef Desc, bool ByDefault = false) {
-    if (ByDefault)
-      Action->setInitialValue(CB);
-    Action->getParser().addLiteralOption(Name, CB, Desc);
-  }
+  Opt(StringRef Name, FnT CB, StringRef Desc, bool ByDefault = false);
 };
 
+/// Convienence wrapper around `Opt` that registers `EmitterClass::run` as the
+/// callback.
 template <class EmitterC> class OptClass : Opt {
   static void run(RecordKeeper &RK, raw_ostream &OS) { EmitterC(RK).run(OS); }
 
@@ -47,6 +43,10 @@ template <class EmitterC> class OptClass : Opt {
   OptClass(StringRef Name, StringRef Desc) : Opt(Name, run, Desc) {}
 };
 
+/// Apply callback for any command line option registered above. Returns false
+/// is no callback was applied.
+bool ApplyCallback(RecordKeeper &Records, raw_ostream &OS);
+
 } // namespace TableGen::Emitter
 
 /// emitSourceFileHeader - Output an LLVM style file header to the specified
@@ -54,6 +54,6 @@ template <class EmitterC> class OptClass : Opt {
 void emitSourceFileHeader(StringRef Desc, raw_ostream &OS,
                           const RecordKeeper &Record = RecordKeeper());
 
-} // End llvm namespace
+} // namespace llvm
 
-#endif
+#endif // LLVM_TABLEGEN_TABLEGENBACKEND_H
diff --git a/llvm/lib/TableGen/Main.cpp b/llvm/lib/TableGen/Main.cpp
index 841fa7c3f3690b..a4c41223c07620 100644
--- a/llvm/lib/TableGen/Main.cpp
+++ b/llvm/lib/TableGen/Main.cpp
@@ -131,13 +131,10 @@ int llvm::TableGenMain(const char *argv0,
   std::string OutString;
   raw_string_ostream Out(OutString);
   unsigned status = 0;
-  TableGen::Emitter::FnT ActionFn = TableGen::Emitter::Action->getValue();
-  if (ActionFn)
-    ActionFn(Records, Out);
-  else if (MainFn)
-    status = MainFn(Out, Records);
-  else
-    return 1;
+  // ApplyCallback will return true if it did not apply any callback. In that
+  // case, attempt to apply the MainFn.
+  if (TableGen::Emitter::ApplyCallback(Records, Out))
+    status = MainFn ? MainFn(Out, Records) : 1;
   Records.stopBackendTimer();
   if (status)
     return 1;
diff --git a/llvm/lib/TableGen/TableGenBackend.cpp b/llvm/lib/TableGen/TableGenBackend.cpp
index 035abe936e114d..f97fcb129e6414 100644
--- a/llvm/lib/TableGen/TableGenBackend.cpp
+++ b/llvm/lib/TableGen/TableGenBackend.cpp
@@ -12,6 +12,8 @@
 
 #include "llvm/TableGen/TableGenBackend.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
@@ -19,15 +21,56 @@
 #include <cstddef>
 
 using namespace llvm;
+using namespace TableGen::Emitter;
 
 const size_t MAX_LINE_LEN = 80U;
 
-namespace llvm::TableGen::Emitter {
-ManagedStatic<cl::opt<FnT>, OptCreatorT> Action;
-void *OptCreatorT::call() {
-  return new cl::opt<FnT>(cl::desc("Action to perform:"));
+// CommandLine options of class type are not directly supported with some
+// specific exceptions like std::string which are safe to copy. In our case,
+// the `FnT` function_ref object is also safe to copy. So provide a
+// specialization of `OptionValue` for `FnT` type that stores it as a copy.
+// This is essentially similar to OptionValue<std::string> specialization for
+// strings.
+template <> struct cl::OptionValue<FnT> final : cl::OptionValueCopy<FnT> {
+  OptionValue() = default;
+
+  OptionValue(const FnT &V) { this->setValue(V); }
+
+  OptionValue<FnT> &operator=(const FnT &V) {
+    setValue(V);
+    return *this;
+  }
+
+private:
+  void anchor() override {}
+};
+
+namespace {
+struct OptCreatorT {
+  static void *call() {
+    return new cl::opt<FnT>(cl::desc("Action to perform:"));
+  }
+};
+} // namespace
+
+static ManagedStatic<cl::opt<FnT>, OptCreatorT> CallbackFunction;
+
+Opt::Opt(StringRef Name, FnT CB, StringRef Desc, bool ByDefault) {
+  if (ByDefault)
+    CallbackFunction->setInitialValue(CB);
+  CallbackFunction->getParser().addLiteralOption(Name, CB, Desc);
+}
+
+/// Apply callback specified on the command line. Returns true if no callback
+/// was applied.
+bool llvm::TableGen::Emitter::ApplyCallback(RecordKeeper &Records,
+                                            raw_ostream &OS) {
+  FnT Fn = CallbackFunction->getValue();
+  if (!Fn)
+    return true;
+  Fn(Records, OS);
+  return false;
 }
-} // namespace llvm::TableGen::Emitter
 
 static void printLine(raw_ostream &OS, const Twine &Prefix, char Fill,
                       StringRef Suffix) {
@@ -59,7 +102,7 @@ void llvm::emitSourceFileHeader(StringRef Desc, raw_ostream &OS,
   printLine(OS, Prefix + "Automatically generated file, do not edit!", ' ',
             Suffix);
 
-  // Print the filename of source file
+  // Print the filename of source file.
   if (!Record.getInputFilename().empty())
     printLine(
         OS, Prefix + "From: " + sys::path::filename(Record.getInputFilename()),
diff --git a/llvm/unittests/ADT/FunctionRefTest.cpp b/llvm/unittests/ADT/FunctionRefTest.cpp
index 47633cada0f3cc..c4a6c9e437c82c 100644
--- a/llvm/unittests/ADT/FunctionRefTest.cpp
+++ b/llvm/unittests/ADT/FunctionRefTest.cpp
@@ -59,4 +59,12 @@ TEST(FunctionRefTest, SFINAE) {
   EXPECT_EQ("string", returns([] { return "hello"; }));
 }
 
+TEST(FunctionRefTest, Equality) {
+  function_ref<int()> X = [] { return 1; };
+  function_ref<int()> Y = [] { return 2; };
+  EXPECT_TRUE(!(X == Y));
+  Y = X;
+  EXPECT_EQ(X, Y);
+}
+
 } // namespace
diff --git a/llvm/utils/TableGen/DisassemblerEmitter.cpp b/llvm/utils/TableGen/DisassemblerEmitter.cpp
index d41750075b41f2..f2c25d38ad2a7d 100644
--- a/llvm/utils/TableGen/DisassemblerEmitter.cpp
+++ b/llvm/utils/TableGen/DisassemblerEmitter.cpp
@@ -11,6 +11,7 @@
 #include "WebAssemblyDisassemblerEmitter.h"
 #include "X86DisassemblerTables.h"
 #include "X86RecognizableInstr.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
 #include "llvm/TableGen/TableGenBackend.h"
diff --git a/llvm/utils/TableGen/IntrinsicEmitter.cpp b/llvm/utils/TableGen/IntrinsicEmitter.cpp
index 5d972157828784..a2b1eb1ed20602 100644
--- a/llvm/utils/TableGen/IntrinsicEmitter.cpp
+++ b/llvm/utils/TableGen/IntrinsicEmitter.cpp
@@ -65,6 +65,14 @@ class IntrinsicEmitter {
   void EmitIntrinsicToBuiltinMap(const CodeGenIntrinsicTable &Ints,
                                  bool IsClang, raw_ostream &OS);
 };
+
+// Helper class to use with `TableGen::Emitter::OptClass`.
+template <bool Enums> class IntrinsicEmitterOpt : public IntrinsicEmitter {
+public:
+  IntrinsicEmitterOpt(const RecordKeeper &R) : IntrinsicEmitter(R) {}
+  void run(raw_ostream &OS) { IntrinsicEmitter::run(OS, Enums); }
+};
+
 } // End anonymous namespace
 
 //===----------------------------------------------------------------------===//
@@ -772,16 +780,8 @@ Intrinsic::getIntrinsicFor{1}Builtin(StringRef TargetPrefix,
                 UpperCompilerName);
 }
 
-static void EmitIntrinsicEnums(RecordKeeper &RK, raw_ostream &OS) {
-  IntrinsicEmitter(RK).run(OS, /*Enums=*/true);
-}
-
-static TableGen::Emitter::Opt X("gen-intrinsic-enums", EmitIntrinsicEnums,
-                                "Generate intrinsic enums");
-
-static void EmitIntrinsicImpl(RecordKeeper &RK, raw_ostream &OS) {
-  IntrinsicEmitter(RK).run(OS, /*Enums=*/false);
-}
+static TableGen::Emitter::OptClass<IntrinsicEmitterOpt</*Enums=*/true>>
+    X("gen-intrinsic-enums", "Generate intrinsic enums");
 
-static TableGen::Emitter::Opt Y("gen-intrinsic-impl", EmitIntrinsicImpl,
-                                "Generate intrinsic implementation code");
+static TableGen::Emitter::OptClass<IntrinsicEmitterOpt</*Enums=*/false>>
+    Y("gen-intrinsic-impl", "Generate intrinsic implementation code");
diff --git a/llvm/utils/TableGen/TableGen.cpp b/llvm/utils/TableGen/TableGen.cpp
index c420843574cbf3..7ee6fa5c832114 100644
--- a/llvm/utils/TableGen/TableGen.cpp
+++ b/llvm/utils/TableGen/TableGen.cpp
@@ -39,7 +39,7 @@ static cl::opt<std::string> Class("class",
                                   cl::value_desc("class name"),
                                   cl::cat(PrintEnumsCat));
 
-static void PrintRecords(RecordKeeper &Records, raw_ostream &OS) {
+static void PrintRecords(const RecordKeeper &Records, raw_ostream &OS) {
   OS << Records; // No argument, dump all contents
 }
 
@@ -49,14 +49,14 @@ static void PrintEnums(RecordKeeper &Records, raw_ostream &OS) {
   OS << "\n";
 }
 
-static void PrintSets(RecordKeeper &Records, raw_ostream &OS) {
+static void PrintSets(const RecordKeeper &Records, raw_ostream &OS) {
   SetTheory Sets;
   Sets.addFieldExpander("Set", "Elements");
   for (Record *Rec : Records.getAllDerivedDefinitions("Set")) {
     OS << Rec->getName() << " = [";
     const std::vector<Record *> *Elts = Sets.expand(Rec);
     assert(Elts && "Couldn't expand Set instance");
-    for (Record *Elt : *Elts)
+    for (const Record *Elt : *Elts)
       OS << ' ' << Elt->getName();
     OS << " ]\n";
   }
@@ -67,7 +67,7 @@ static TableGen::Emitter::Opt X[] = {
      true},
     {"print-detailed-records", EmitDetailedRecords,
      "Print full details of all records to stdout"},
-    {"null-backend", [](RecordKeeper &Records, raw_ostream &OS) {},
+    {"null-backend", [](const RecordKeeper &Records, raw_ostream &OS) {},
      "Do nothing after parsing (useful for timing)"},
     {"dump-json", EmitJSON, "Dump all records as machine-readable JSON"},
     {"print-enums", PrintEnums, "Print enum values for a class"},

llvm/lib/TableGen/TableGenBackend.cpp

llvm/unittests/ADT/FunctionRefTest.cpp

Pierre-vh · 2024-08-23T11:09:55Z

llvm/include/llvm/ADT/STLFunctionalExtras.h

@@ -69,6 +69,10 @@ class function_ref<Ret(Params...)> {
  }

  explicit operator bool() const { return callback; }
+
+  bool operator==(const function_ref<Ret(Params...)> &Other) const {


Why is this needed now?

This is needed now because of its use in cl::OptionValueCopy<FnT>, which uses the == operator.

- Refactor TableGen backend options to allow specifying a callback function that takes a const reference to `RecordKeeper`. This will enable gradual migration of code to use const references and pointers to `RecordKeeper` and `Record` in the TableGen backend. Add support for both forms of the callback to both `Opt` and `OptClass`. - Refactor handling of the callback command line options. Move variable for the command line option from the header to the CPP file, and add a function `ApplyCallback` to apply the selected callback. - Change callbacks in TableGen.cpp to take const reference. They use the `Opt` class to define their callbacks. - Change IntrinsicEmitter to use the `OptClass` to define its callbacks. It already uses a const reference.

Pierre-vh

LGTM but please wait a working day or two to see if anyone else has concerns

jurahul · 2024-08-23T13:13:37Z

LGTM but please wait a working day or two to see if anyone else has concerns

Awesome, yeah I'll wait till next week to merge it given the weekend is approaching.

Change TableGen backend callback function to require a const RecordKeeper argument (by changing it from function_ref to just a function pointer). This undoes parts of llvm#104716 which was added to enable gradual migration of TableGen backends to use const RecordKeeper (by allowing either const or non-const references). Now that all backends have been migrated to const reference, we do not need this.

…1064) Change TableGen backend callback function to require a const RecordKeeper argument (by changing it from function_ref to just a function pointer). This undoes parts of #104716 which was added to enable gradual migration of TableGen backends to use const RecordKeeper (by allowing either const or non-const references). Now that all backends have been migrated to const reference, we do not need this. This is a part of effort to have better const correctness in TableGen backends: https://discourse.llvm.org/t/psa-planned-changes-to-tablegen-getallderiveddefinitions-api-potential-downstream-breakages/81089

jurahul force-pushed the tg_emitter_action_constref branch 3 times, most recently from e54fc66 to 45fc41b Compare August 20, 2024 17:54

jurahul changed the title ~~[TableGen] Allow emitter actions to use const RecordKeeper &~~ [TableGen] Allow emitter callbacks to use const RecordKeeper & Aug 20, 2024

jurahul requested review from chapuni, Pierre-vh and mshockwave August 20, 2024 20:23

jurahul marked this pull request as ready for review August 20, 2024 20:25

mshockwave reviewed Aug 20, 2024

View reviewed changes

llvm/utils/TableGen/TableGen.cpp Outdated Show resolved Hide resolved

Pierre-vh reviewed Aug 21, 2024

View reviewed changes

llvm/include/llvm/TableGen/TableGenBackend.h Outdated Show resolved Hide resolved

llvm/lib/TableGen/TableGenBackend.cpp Outdated Show resolved Hide resolved

jurahul force-pushed the tg_emitter_action_constref branch from 45fc41b to 7c0eb46 Compare August 22, 2024 02:25

llvmbot added the llvm:adt label Aug 22, 2024

jurahul marked this pull request as draft August 22, 2024 02:26

jurahul marked this pull request as ready for review August 22, 2024 12:23

jurahul requested review from mshockwave and Pierre-vh August 23, 2024 10:54

Pierre-vh reviewed Aug 23, 2024

View reviewed changes

jurahul force-pushed the tg_emitter_action_constref branch from 7c0eb46 to c58dfca Compare August 23, 2024 13:09

jurahul requested a review from Pierre-vh August 23, 2024 13:11

Pierre-vh approved these changes Aug 23, 2024

View reviewed changes

jurahul mentioned this pull request Aug 26, 2024

[NFC][TableGen] Refactor JSON and detailed record emitter #105770

Merged

jurahul merged commit 34dee0a into llvm:main Aug 26, 2024
6 of 8 checks passed

jurahul deleted the tg_emitter_action_constref branch August 26, 2024 19:10

jurahul mentioned this pull request Oct 3, 2024

[TableGen] Change backend callback to require const RecordKeeper #111064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TableGen] Allow emitter callbacks to use `const RecordKeeper &` #104716

[TableGen] Allow emitter callbacks to use `const RecordKeeper &` #104716

Uh oh!

jurahul commented Aug 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Aug 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pierre-vh Aug 23, 2024

Uh oh!

jurahul Aug 23, 2024

Uh oh!

Pierre-vh left a comment

Uh oh!

jurahul commented Aug 23, 2024

Uh oh!

Uh oh!

Uh oh!

[TableGen] Allow emitter callbacks to use const RecordKeeper & #104716

[TableGen] Allow emitter callbacks to use const RecordKeeper & #104716

Uh oh!

Conversation

jurahul commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Aug 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pierre-vh Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

jurahul Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

Pierre-vh left a comment

Choose a reason for hiding this comment

Uh oh!

jurahul commented Aug 23, 2024

Uh oh!

Uh oh!

Uh oh!

[TableGen] Allow emitter callbacks to use `const RecordKeeper &` #104716

[TableGen] Allow emitter callbacks to use `const RecordKeeper &` #104716

jurahul commented Aug 18, 2024 •

edited

Loading