[TableGen] Add const variants of accessors for backend #106658
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jurahul
force-pushed
the
const_record
branch
2 times, most recently
from
5ee1d2e
to
ac32657
Compare
|
Note, this is related to the following discourse thread that I had started: https://discourse.llvm.org/t/changing-tablegen-getallderiveddefinitions-to-return-arrayref-const-record/80586/5 |
llvmbot
added
clang
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-clang Author: Rahul Joshi (jurahul) ChangesSplit RecordKeeper This will help gradual migration of TableGen backends to use Patch is 31.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106658.diff 19 Files Affected:
diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp
index adbe6af62d5cbe..d24215d10f17c7 100644
--- a/clang/utils/TableGen/ClangAttrEmitter.cpp
+++ b/clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -189,7 +189,7 @@ static StringRef NormalizeGNUAttrSpelling(StringRef AttrSpelling) {
typedef std::vector<std::pair<std::string, const Record *>> ParsedAttrMap;
-static ParsedAttrMap getParsedAttrList(const RecordKeeper &Records,
+static ParsedAttrMap getParsedAttrList(RecordKeeper &Records,
ParsedAttrMap *Dupes = nullptr,
bool SemaOnly = true) {
std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr");
@@ -4344,7 +4344,7 @@ static void GenerateAppertainsTo(const Record &Attr, raw_ostream &OS) {
// written into OS and the checks for merging declaration attributes are
// written into MergeOS.
static void GenerateMutualExclusionsChecks(const Record &Attr,
- const RecordKeeper &Records,
+ RecordKeeper &Records,
raw_ostream &OS,
raw_ostream &MergeDeclOS,
raw_ostream &MergeStmtOS) {
diff --git a/clang/utils/TableGen/ClangSyntaxEmitter.cpp b/clang/utils/TableGen/ClangSyntaxEmitter.cpp
index 9720d587318432..2a69e4c353b6b4 100644
--- a/clang/utils/TableGen/ClangSyntaxEmitter.cpp
+++ b/clang/utils/TableGen/ClangSyntaxEmitter.cpp
@@ -41,7 +41,7 @@ using llvm::formatv;
// stable and useful way, where abstract Node subclasses correspond to ranges.
class Hierarchy {
public:
- Hierarchy(const llvm::RecordKeeper &Records) {
+ Hierarchy(llvm::RecordKeeper &Records) {
for (llvm::Record *T : Records.getAllDerivedDefinitions("NodeType"))
add(T);
for (llvm::Record *Derived : Records.getAllDerivedDefinitions("NodeType"))
diff --git a/llvm/include/llvm/TableGen/DirectiveEmitter.h b/llvm/include/llvm/TableGen/DirectiveEmitter.h
index 1121459be6ce7d..ca21c8fc101450 100644
--- a/llvm/include/llvm/TableGen/DirectiveEmitter.h
+++ b/llvm/include/llvm/TableGen/DirectiveEmitter.h
@@ -15,8 +15,7 @@ namespace llvm {
// DirectiveBase.td and provides helper methods for accessing it.
class DirectiveLanguage {
public:
- explicit DirectiveLanguage(const llvm::RecordKeeper &Records)
- : Records(Records) {
+ explicit DirectiveLanguage(llvm::RecordKeeper &Records) : Records(Records) {
const auto &DirectiveLanguages = getDirectiveLanguages();
Def = DirectiveLanguages[0];
}
@@ -71,7 +70,7 @@ class DirectiveLanguage {
private:
const llvm::Record *Def;
- const llvm::RecordKeeper &Records;
+ llvm::RecordKeeper &Records;
std::vector<Record *> getDirectiveLanguages() const {
return Records.getAllDerivedDefinitions("DirectiveLanguage");
diff --git a/llvm/include/llvm/TableGen/Record.h b/llvm/include/llvm/TableGen/Record.h
index a339946e67cf2d..b3b0ee601be4f4 100644
--- a/llvm/include/llvm/TableGen/Record.h
+++ b/llvm/include/llvm/TableGen/Record.h
@@ -2057,19 +2057,28 @@ class RecordKeeper {
//===--------------------------------------------------------------------===//
// High-level helper methods, useful for tablegen backends.
+ // Non-const methods return std::vector<Record *> by value or reference.
+ // Const methods return std::vector<const Record *> by value or reference.
+
/// Get all the concrete records that inherit from the one specified
/// class. The class must be defined.
- std::vector<Record *> getAllDerivedDefinitions(StringRef ClassName) const;
+ const std::vector<const Record *> &
+ getAllDerivedDefinitions(StringRef ClassName) const;
+ const std::vector<Record *> &getAllDerivedDefinitions(StringRef ClassName);
/// Get all the concrete records that inherit from all the specified
/// classes. The classes must be defined.
- std::vector<Record *> getAllDerivedDefinitions(
- ArrayRef<StringRef> ClassNames) const;
+ std::vector<const Record *>
+ getAllDerivedDefinitions(ArrayRef<StringRef> ClassNames) const;
+ std::vector<Record *>
+ getAllDerivedDefinitions(ArrayRef<StringRef> ClassNames);
/// Get all the concrete records that inherit from specified class, if the
/// class is defined. Returns an empty vector if the class is not defined.
- std::vector<Record *>
+ const std::vector<const Record *> &
getAllDerivedDefinitionsIfDefined(StringRef ClassName) const;
+ const std::vector<Record *> &
+ getAllDerivedDefinitionsIfDefined(StringRef ClassName);
void dump() const;
@@ -2079,9 +2088,24 @@ class RecordKeeper {
RecordKeeper &operator=(RecordKeeper &&) = delete;
RecordKeeper &operator=(const RecordKeeper &) = delete;
+ // Helper template functions for backend accessors.
+ template <typename VecTy>
+ const VecTy &
+ getAllDerivedDefinitionsImpl(StringRef ClassName,
+ std::map<std::string, VecTy> &Cache) const;
+
+ template <typename VecTy>
+ VecTy getAllDerivedDefinitionsImpl(ArrayRef<StringRef> ClassNames) const;
+
+ template <typename VecTy>
+ const VecTy &getAllDerivedDefinitionsIfDefinedImpl(
+ StringRef ClassName, std::map<std::string, VecTy> &Cache) const;
+
std::string InputFilename;
RecordMap Classes, Defs;
- mutable StringMap<std::vector<Record *>> ClassRecordsMap;
+ mutable std::map<std::string, std::vector<const Record *>>
+ ClassRecordsMapConst;
+ mutable std::map<std::string, std::vector<Record *>> ClassRecordsMap;
GlobalMap ExtraGlobals;
// These members are for the phase timing feature. We need a timer group,
diff --git a/llvm/lib/TableGen/Record.cpp b/llvm/lib/TableGen/Record.cpp
index bcecee8e550c88..eeb7ac2dfa138d 100644
--- a/llvm/lib/TableGen/Record.cpp
+++ b/llvm/lib/TableGen/Record.cpp
@@ -3219,25 +3219,28 @@ void RecordKeeper::stopBackendTimer() {
}
}
-std::vector<Record *>
-RecordKeeper::getAllDerivedDefinitions(StringRef ClassName) const {
+template <typename VecTy>
+const VecTy &RecordKeeper::getAllDerivedDefinitionsImpl(
+ StringRef ClassName, std::map<std::string, VecTy> &Cache) const {
// We cache the record vectors for single classes. Many backends request
// the same vectors multiple times.
- auto Pair = ClassRecordsMap.try_emplace(ClassName);
+ auto Pair = Cache.try_emplace(ClassName.str());
if (Pair.second)
- Pair.first->second = getAllDerivedDefinitions(ArrayRef(ClassName));
+ Pair.first->second =
+ getAllDerivedDefinitionsImpl<VecTy>(ArrayRef(ClassName));
return Pair.first->second;
}
-std::vector<Record *> RecordKeeper::getAllDerivedDefinitions(
+template <typename VecTy>
+VecTy RecordKeeper::getAllDerivedDefinitionsImpl(
ArrayRef<StringRef> ClassNames) const {
- SmallVector<Record *, 2> ClassRecs;
- std::vector<Record *> Defs;
+ SmallVector<const Record *, 2> ClassRecs;
+ VecTy Defs;
assert(ClassNames.size() > 0 && "At least one class must be passed.");
for (const auto &ClassName : ClassNames) {
- Record *Class = getClass(ClassName);
+ const Record *Class = getClass(ClassName);
if (!Class)
PrintFatalError("The class '" + ClassName + "' is not defined\n");
ClassRecs.push_back(Class);
@@ -3245,20 +3248,54 @@ std::vector<Record *> RecordKeeper::getAllDerivedDefinitions(
for (const auto &OneDef : getDefs()) {
if (all_of(ClassRecs, [&OneDef](const Record *Class) {
- return OneDef.second->isSubClassOf(Class);
- }))
+ return OneDef.second->isSubClassOf(Class);
+ }))
Defs.push_back(OneDef.second.get());
}
-
llvm::sort(Defs, LessRecord());
-
return Defs;
}
+template <typename VecTy>
+const VecTy &RecordKeeper::getAllDerivedDefinitionsIfDefinedImpl(
+ StringRef ClassName, std::map<std::string, VecTy> &Cache) const {
+ return getClass(ClassName)
+ ? getAllDerivedDefinitionsImpl<VecTy>(ClassName, Cache)
+ : Cache[""];
+}
+
+const std::vector<const Record *> &
+RecordKeeper::getAllDerivedDefinitions(StringRef ClassName) const {
+ return getAllDerivedDefinitionsImpl<std::vector<const Record *>>(
+ ClassName, ClassRecordsMapConst);
+}
+
+const std::vector<Record *> &
+RecordKeeper::getAllDerivedDefinitions(StringRef ClassName) {
+ return getAllDerivedDefinitionsImpl<std::vector<Record *>>(ClassName,
+ ClassRecordsMap);
+}
+
+std::vector<const Record *>
+RecordKeeper::getAllDerivedDefinitions(ArrayRef<StringRef> ClassNames) const {
+ return getAllDerivedDefinitionsImpl<std::vector<const Record *>>(ClassNames);
+}
+
std::vector<Record *>
+RecordKeeper::getAllDerivedDefinitions(ArrayRef<StringRef> ClassNames) {
+ return getAllDerivedDefinitionsImpl<std::vector<Record *>>(ClassNames);
+}
+
+const std::vector<const Record *> &
RecordKeeper::getAllDerivedDefinitionsIfDefined(StringRef ClassName) const {
- return getClass(ClassName) ? getAllDerivedDefinitions(ClassName)
- : std::vector<Record *>();
+ return getAllDerivedDefinitionsIfDefinedImpl<std::vector<const Record *>>(
+ ClassName, ClassRecordsMapConst);
+}
+
+const std::vector<Record *> &
+RecordKeeper::getAllDerivedDefinitionsIfDefined(StringRef ClassName) {
+ return getAllDerivedDefinitionsIfDefinedImpl<std::vector<Record *>>(
+ ClassName, ClassRecordsMap);
}
Init *MapResolver::resolve(Init *VarName) {
diff --git a/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp b/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
index 4bca904e9f38bd..6ffe788bcebca2 100644
--- a/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
+++ b/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
@@ -26,15 +26,13 @@ using namespace llvm;
//===----------------------------------------------------------------------===//
CodeGenIntrinsicTable::CodeGenIntrinsicTable(const RecordKeeper &RC) {
- std::vector<Record *> IntrProperties =
- RC.getAllDerivedDefinitions("IntrinsicProperty");
-
std::vector<const Record *> DefaultProperties;
- for (const Record *Rec : IntrProperties)
+ for (const Record *Rec : RC.getAllDerivedDefinitions("IntrinsicProperty"))
if (Rec->getValueAsBit("IsDefault"))
DefaultProperties.push_back(Rec);
- std::vector<Record *> Defs = RC.getAllDerivedDefinitions("Intrinsic");
+ const std::vector<const Record *> &Defs =
+ RC.getAllDerivedDefinitions("Intrinsic");
Intrinsics.reserve(Defs.size());
for (const Record *Def : Defs)
diff --git a/llvm/utils/TableGen/Common/SubtargetFeatureInfo.cpp b/llvm/utils/TableGen/Common/SubtargetFeatureInfo.cpp
index 4f57234d6fe275..a4d6d8d21b3562 100644
--- a/llvm/utils/TableGen/Common/SubtargetFeatureInfo.cpp
+++ b/llvm/utils/TableGen/Common/SubtargetFeatureInfo.cpp
@@ -21,7 +21,7 @@ LLVM_DUMP_METHOD void SubtargetFeatureInfo::dump() const {
#endif
std::vector<std::pair<Record *, SubtargetFeatureInfo>>
-SubtargetFeatureInfo::getAll(const RecordKeeper &Records) {
+SubtargetFeatureInfo::getAll(RecordKeeper &Records) {
std::vector<std::pair<Record *, SubtargetFeatureInfo>> SubtargetFeatures;
std::vector<Record *> AllPredicates =
Records.getAllDerivedDefinitions("Predicate");
diff --git a/llvm/utils/TableGen/Common/SubtargetFeatureInfo.h b/llvm/utils/TableGen/Common/SubtargetFeatureInfo.h
index 2635e4b733e1a3..fee2c0263c4960 100644
--- a/llvm/utils/TableGen/Common/SubtargetFeatureInfo.h
+++ b/llvm/utils/TableGen/Common/SubtargetFeatureInfo.h
@@ -49,7 +49,7 @@ struct SubtargetFeatureInfo {
void dump() const;
static std::vector<std::pair<Record *, SubtargetFeatureInfo>>
- getAll(const RecordKeeper &Records);
+ getAll(RecordKeeper &Records);
/// Emit the subtarget feature flag definitions.
///
diff --git a/llvm/utils/TableGen/ExegesisEmitter.cpp b/llvm/utils/TableGen/ExegesisEmitter.cpp
index d48c7f3a480f24..0de7cb42337481 100644
--- a/llvm/utils/TableGen/ExegesisEmitter.cpp
+++ b/llvm/utils/TableGen/ExegesisEmitter.cpp
@@ -59,7 +59,7 @@ class ExegesisEmitter {
};
static std::map<llvm::StringRef, unsigned>
-collectPfmCounters(const RecordKeeper &Records) {
+collectPfmCounters(RecordKeeper &Records) {
std::map<llvm::StringRef, unsigned> PfmCounterNameTable;
const auto AddPfmCounterName = [&PfmCounterNameTable](
const Record *PfmCounterDef) {
diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp
index a491a049e7c812..2606768c0c582c 100644
--- a/llvm/utils/TableGen/GlobalISelEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp
@@ -335,7 +335,7 @@ class GlobalISelEmitter final : public GlobalISelMatchTableExecutorEmitter {
private:
std::string ClassName;
- const RecordKeeper &RK;
+ RecordKeeper &RK;
const CodeGenDAGPatterns CGP;
const CodeGenTarget &Target;
CodeGenRegBank &CGRegs;
diff --git a/llvm/utils/TableGen/SubtargetEmitter.cpp b/llvm/utils/TableGen/SubtargetEmitter.cpp
index 66ca38ee5ae2f8..7ae61cb7c446b1 100644
--- a/llvm/utils/TableGen/SubtargetEmitter.cpp
+++ b/llvm/utils/TableGen/SubtargetEmitter.cpp
@@ -1545,7 +1545,7 @@ void SubtargetEmitter::EmitSchedModel(raw_ostream &OS) {
EmitProcessorModels(OS);
}
-static void emitPredicateProlog(const RecordKeeper &Records, raw_ostream &OS) {
+static void emitPredicateProlog(RecordKeeper &Records, raw_ostream &OS) {
std::string Buffer;
raw_string_ostream Stream(Buffer);
diff --git a/llvm/utils/TableGen/TableGen.cpp b/llvm/utils/TableGen/TableGen.cpp
index 7ee6fa5c832114..882410bac081b4 100644
--- a/llvm/utils/TableGen/TableGen.cpp
+++ b/llvm/utils/TableGen/TableGen.cpp
@@ -52,7 +52,7 @@ static void PrintEnums(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")) {
+ for (const Record *Rec : Records.getAllDerivedDefinitions("Set")) {
OS << Rec->getName() << " = [";
const std::vector<Record *> *Elts = Sets.expand(Rec);
assert(Elts && "Couldn't expand Set instance");
diff --git a/mlir/include/mlir/TableGen/GenInfo.h b/mlir/include/mlir/TableGen/GenInfo.h
index ef2e12f07df16d..d59d64223827bd 100644
--- a/mlir/include/mlir/TableGen/GenInfo.h
+++ b/mlir/include/mlir/TableGen/GenInfo.h
@@ -21,8 +21,8 @@ class RecordKeeper;
namespace mlir {
/// Generator function to invoke.
-using GenFunction = std::function<bool(const llvm::RecordKeeper &recordKeeper,
- raw_ostream &os)>;
+using GenFunction =
+ std::function<bool(llvm::RecordKeeper &recordKeeper, raw_ostream &os)>;
/// Structure to group information about a generator (argument to invoke via
/// mlir-tblgen, description, and generator function).
@@ -34,7 +34,7 @@ class GenInfo {
: arg(arg), description(description), generator(std::move(generator)) {}
/// Invokes the generator and returns whether the generator failed.
- bool invoke(const llvm::RecordKeeper &recordKeeper, raw_ostream &os) const {
+ bool invoke(llvm::RecordKeeper &recordKeeper, raw_ostream &os) const {
assert(generator && "Cannot call generator with null generator");
return generator(recordKeeper, os);
}
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index eccd8029d950ff..feca04bff643d5 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -690,10 +690,10 @@ class DefGenerator {
bool emitDefs(StringRef selectedDialect);
protected:
- DefGenerator(std::vector<llvm::Record *> &&defs, raw_ostream &os,
+ DefGenerator(const std::vector<llvm::Record *> &defs, raw_ostream &os,
StringRef defType, StringRef valueType, bool isAttrGenerator)
- : defRecords(std::move(defs)), os(os), defType(defType),
- valueType(valueType), isAttrGenerator(isAttrGenerator) {
+ : defRecords(defs), os(os), defType(defType), valueType(valueType),
+ isAttrGenerator(isAttrGenerator) {
// Sort by occurrence in file.
llvm::sort(defRecords, [](llvm::Record *lhs, llvm::Record *rhs) {
return lhs->getID() < rhs->getID();
@@ -721,13 +721,13 @@ class DefGenerator {
/// A specialized generator for AttrDefs.
struct AttrDefGenerator : public DefGenerator {
- AttrDefGenerator(const llvm::RecordKeeper &records, raw_ostream &os)
+ AttrDefGenerator(llvm::RecordKeeper &records, raw_ostream &os)
: DefGenerator(records.getAllDerivedDefinitionsIfDefined("AttrDef"), os,
"Attr", "Attribute", /*isAttrGenerator=*/true) {}
};
/// A specialized generator for TypeDefs.
struct TypeDefGenerator : public DefGenerator {
- TypeDefGenerator(const llvm::RecordKeeper &records, raw_ostream &os)
+ TypeDefGenerator(llvm::RecordKeeper &records, raw_ostream &os)
: DefGenerator(records.getAllDerivedDefinitionsIfDefined("TypeDef"), os,
"Type", "Type", /*isAttrGenerator=*/false) {}
};
@@ -1029,7 +1029,7 @@ bool DefGenerator::emitDefs(StringRef selectedDialect) {
/// Find all type constraints for which a C++ function should be generated.
static std::vector<Constraint>
-getAllTypeConstraints(const llvm::RecordKeeper &records) {
+getAllTypeConstraints(llvm::RecordKeeper &records) {
std::vector<Constraint> result;
for (llvm::Record *def :
records.getAllDerivedDefinitionsIfDefined("TypeConstraint")) {
@@ -1046,7 +1046,7 @@ getAllTypeConstraints(const llvm::RecordKeeper &records) {
return result;
}
-static void emitTypeConstraintDecls(const llvm::RecordKeeper &records,
+static void emitTypeConstraintDecls(llvm::RecordKeeper &records,
raw_ostream &os) {
static const char *const typeConstraintDecl = R"(
bool {0}(::mlir::Type type);
@@ -1056,7 +1056,7 @@ bool {0}(::mlir::Type type);
os << strfmt(typeConstraintDecl, *constr.getCppFunctionName());
}
-static void emitTypeConstraintDefs(const llvm::RecordKeeper &records,
+static void emitTypeConstraintDefs(llvm::RecordKeeper &records,
raw_ostream &os) {
static const char *const typeConstraintDef = R"(
bool {0}(::mlir::Type type) {
@@ -1087,13 +1087,13 @@ static llvm::cl::opt<std::string>
static mlir::GenRegistration
genAttrDefs("gen-attrdef-defs", "Generate AttrDef definitions",
- [](const llvm::RecordKeeper &records, raw_ostream &os) {
+ [](llvm::RecordKeeper &records, raw_ostream &os) {
AttrDefGenerator generator(records, os);
return generator.emitDefs(attrDialect);
});
static mlir::GenRegistration
genAttrDecls("gen-attrdef-decls", "Generate AttrDef declarations",
- [](const llvm::RecordKeeper &records, raw_ostream &os) {
+ [](llvm::RecordKeeper &records, raw_ostream &os) {
AttrDefGenerator generator(records, os);
return generator.emitDecls(attrDialect);
});
@@ -1109,13 +1109,13 @@ static llvm::cl::opt<std::string>
static mlir::GenRegistration
genTypeDefs("gen-typedef-defs", "Generate TypeDef definitions",
- [](const llvm::RecordKeeper &records, raw_ostream &os) {
+ [](llvm::RecordKeeper &records, raw_ostream &os) {
TypeDefGenerator generator(records, os);
return generator.emitDefs(typeDialect);
});
static mlir::GenRegistration
genTypeDecls("gen-typedef-decls", "Generate TypeDef declarations",
- [](const llvm::RecordKeeper &records, raw_ostream &os) {
+ [](llvm::RecordKeeper &records, raw_ostream &os) {
TypeDefGenerator generator(records, os);
return generator.emitDecls(typeDialect);
});
@@ -1123,14 +1123,14 @@ static mlir::GenRegistration
static mlir::GenRegistration
genTypeConstrDefs("gen-type-constraint-defs",
"Generate type constraint definitions",
- [](const llvm::RecordKeeper &records, raw_ostream &os) {
+ [](llvm::RecordKeeper &records, raw_ostream &os) {
emitTypeConstraintD...
[truncated]
|
| @@ -189,7 +189,7 @@ static StringRef NormalizeGNUAttrSpelling(StringRef AttrSpelling) { | |||
|
|
|||
| typedef std::vector<std::pair<std::string, const Record *>> ParsedAttrMap; | |||
|
|
|||
| static ParsedAttrMap getParsedAttrList(const RecordKeeper &Records, | |||
There was a problem hiding this comment.
I expected to see more const instead of less. Why is this here (and below) passes as non-const?
There was a problem hiding this comment.
Please see the discourse thread: https://discourse.llvm.org/t/changing-tablegen-getallderiveddefinitions-to-return-arrayref-const-record/80586/5
Changing all backends to use const in a single go will be too large a change, so this is a way to stage it. Over time, all backends should move to const variant, and then the non-const ones can be deleted.
There was a problem hiding this comment.
Would be good to have those conversions queued up to see how this progresses towards that goal
There was a problem hiding this comment.
I will work on some of them, but not sure if I have the bandwidth to address all of them (Maybe the first few might be more involved and then we will have a better idea of the scope). These will be in similar vein to what I have already done for the IntrinsicEmitter and JSON/Detailed record emitters.
There was a problem hiding this comment.
I propose the following next steps (once this is committed):
- We need to send a PSA as current downstream backends may break due to this (fix is easy)
- I'll work on migrating one of each MLIR. Clang, and may be another LLVM backend to use const.
- For the rest, we need to chip away. May be we try to recruit volunteers?
- Before we can deprecate the non-const functions completely, give enough time for downstream backends to change. Maybe this should be a part of the PSA in (1).
- Delete the non-const functions here, and any other const vs non-const difference we may have introduced during the transition.
There was a problem hiding this comment.
Here's an example of migrating one MLIR backend: 8ffa5e9
There was a problem hiding this comment.
And here's for the entire MLIR tablegen code: 061076d
So may be its not that involved. Note that this does not address all the const correctness issues, as const Record* member function may themselves return non-const pointers. So that needs to be fixed gradually as well. So I propose we get this commit in, and then the MLIR one as a follow on. I will see if I can do the clang one as well.
There was a problem hiding this comment.
And for all clang-tablegen its here: abf19e5
So looks like we may be able to migrate quickly, may be wait for a few days (weeks?) for downstream adoption and delete the non-const overloads.
matthias-springer
approved these changes
Sep 5, 2024
jurahul
force-pushed
the
const_record
branch
from
ac32657
to
664c9fa
Compare
mshockwave
approved these changes
Sep 5, 2024
Yeah, once all the upstream backends are migrated, we should mark the non-const ones deprecated. As for other backends, thanks for the offer! I have diffs for clang and mlir, so I am hoping I can do any other remaining ones as well (lldb and libc seem to have one). But if its start to get delayed, I can ping you for help. |
|
I also did a minor update to the diff: I changed the const versions that used to return references to vectors to instead return |
Split RecordKeeper `getAllDerivedDefinitions` family of functions into two variants: (a) non-const ones that return vectors of `Record *` and (b) const ones, that return vectors of `const Record *`. This will help gradual migration of backend to use `const RecordKeeper` and by implication change code to work with const pointers.
jurahul
force-pushed
the
const_record
branch
from
664c9fa
to
4e1d789
Compare
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Sep 12, 2024
Split RecordKeeper `getAllDerivedDefinitions` family of functions into two variants: (a) non-const ones that return vectors of `Record *` and (b) const ones, that return vector/ArrayRef of `const Record *`. This will help gradual migration of TableGen backends to use `const RecordKeeper` and by implication change code to work with const pointers and better const correctness. Existing backends are not yet compatible with the const family of functions, so change them to use a non-constant `RecordKeeper` reference, till they are migrated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Split RecordKeeper
getAllDerivedDefinitionsfamily of functions into two variants:(a) non-const ones that return vectors of
Record *and(b) const ones, that return vector/ArrayRef of
const Record *.This will help gradual migration of TableGen backends to use
const RecordKeeperand by implication change code to workwith const pointers and better const correctness.
Existing backends are not yet compatible with the const family of
functions, so change them to use a non-constant
RecordKeeperreference, till they are migrated.