[NFC][InstrInfoEmitter] Include location of inst definition in comment #156927
Conversation
Print the source location of the instruction definition in comment next to the enum value for each instruction. To make this more readable, change formatting of the instruction enums to be better aligned.
9a5b639 to
bd041cf
Compare
|
How much file size does this add? These files are already pretty large. We removed comments from *GenDAGISel.inc by default years ago because they had a noticeable effect on compile time. |
|
I didn't check because I didn't think it might be an issue. I can measure and post it here. |
|
I guess also that this enum in particular is available throughout a target backend, so its included multiple times (once per .cpp file for that target) |
So average is a 20% increase in the size. |
|
@llvm/pr-subscribers-tablegen Author: Rahul Joshi (jurahul) ChangesPrint the source location of the instruction definition in comment next to the enum value for each instruction. To make this more readable, change formatting of the instruction enums to be better aligned. Example output: Full diff: https://github.com/llvm/llvm-project/pull/156927.diff 2 Files Affected:
diff --git a/llvm/utils/TableGen/Common/CodeGenInstruction.h b/llvm/utils/TableGen/Common/CodeGenInstruction.h
index 9372614f26e1a..ed0bfa7098eb7 100644
--- a/llvm/utils/TableGen/Common/CodeGenInstruction.h
+++ b/llvm/utils/TableGen/Common/CodeGenInstruction.h
@@ -320,6 +320,8 @@ class CodeGenInstruction {
return RV && isa<DagInit>(RV->getValue());
}
+ StringRef getName() const { return TheDef->getName(); }
+
private:
bool isOperandImpl(StringRef OpListName, unsigned i,
StringRef PropertyName) const;
diff --git a/llvm/utils/TableGen/InstrInfoEmitter.cpp b/llvm/utils/TableGen/InstrInfoEmitter.cpp
index 26d93fc13c9ba..afbd64e4c7012 100644
--- a/llvm/utils/TableGen/InstrInfoEmitter.cpp
+++ b/llvm/utils/TableGen/InstrInfoEmitter.cpp
@@ -25,6 +25,8 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/Casting.h"
+#include "llvm/Support/FormattedStream.h"
+#include "llvm/Support/SourceMgr.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/TableGen/Error.h"
#include "llvm/TableGen/Record.h"
@@ -1289,8 +1291,9 @@ void InstrInfoEmitter::emitRecord(
// emitEnums - Print out enum values for all of the instructions.
void InstrInfoEmitter::emitEnums(
- raw_ostream &OS,
+ raw_ostream &RawOS,
ArrayRef<const CodeGenInstruction *> NumberedInstructions) {
+ formatted_raw_ostream OS(RawOS);
OS << "#ifdef GET_INSTRINFO_ENUM\n";
OS << "#undef GET_INSTRINFO_ENUM\n";
@@ -1302,18 +1305,29 @@ void InstrInfoEmitter::emitEnums(
OS << "namespace llvm::" << Namespace << " {\n";
+ auto II = llvm::max_element(
+ NumberedInstructions,
+ [](const CodeGenInstruction *InstA, const CodeGenInstruction *InstB) {
+ return InstA->getName().size() < InstB->getName().size();
+ });
+ size_t MaxNameSize = (*II)->getName().size();
+
OS << " enum {\n";
- for (const CodeGenInstruction *Inst : NumberedInstructions)
- OS << " " << Inst->TheDef->getName()
- << "\t= " << Target.getInstrIntValue(Inst->TheDef) << ",\n";
+ for (const CodeGenInstruction *Inst : NumberedInstructions) {
+ OS << " " << Inst->TheDef->getName();
+ OS.PadToColumn(MaxNameSize + 5);
+ OS << " = " << Target.getInstrIntValue(Inst->TheDef) << ", // (";
+ OS << SrcMgr.getFormattedLocationNoOffset(Inst->TheDef->getLoc().front())
+ << ")\n";
+ }
OS << " INSTRUCTION_LIST_END = " << NumberedInstructions.size() << '\n';
- OS << " };\n\n";
+ OS << " };\n";
OS << "} // end namespace llvm::" << Namespace << '\n';
OS << "#endif // GET_INSTRINFO_ENUM\n\n";
OS << "#ifdef GET_INSTRINFO_SCHED_ENUM\n";
OS << "#undef GET_INSTRINFO_SCHED_ENUM\n";
- OS << "namespace llvm::" << Namespace << "::Sched {\n\n";
+ OS << "namespace llvm::" << Namespace << "::Sched {\n";
OS << " enum {\n";
auto ExplictClasses = SchedModels.explicitSchedClasses();
for (const auto &[Idx, Class] : enumerate(ExplictClasses))
|
|
How big a concern is this? Or is it one of those deaths by a thousand cuts cases adding to the incremental creep? |
Probably just death by a thousand cuts. There appear to be several other silly things we could remove in this file to mitigate if need to. I've never had much trouble finding instructions with grep on the targets I've worked on, but maybe other targets are worse? It's digging the classes and multiclasses needed to understand an instruction that I usually struggle with. |
I do |
Right, though not all folks may be familiar with this and its easier and more readily available if its right there.
+1, esp if these are auto-generated (which I am sure a lot of downstream backends do). |
That command doesn’t print any location information anyway. |
Right, you need --print-detailed-records to get the location |
|
To be clear, I have no objections to this change. |
Print the source location of the instruction definition in comment next to the enum value for each instruction. To make this more readable, change formatting of the instruction enums to be better aligned.
Example output: