[PATCH] D141135: [RFC][GlobalISel] Replace the current GlobalISel matcher with a bottom-up matcher
Pierre van Houtryve via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 9 01:32:53 PST 2023
Pierre-vh added a comment.
Note: I didn't quite review the algorithm/logic in itself (I'll come back to it later when I'm sure I understand it fully to make meaningful comments), so I focused mostly on code style and general comments
> The old matcher generator implementation is still there, but not used.
I would add a follow-up diff to remove it entirely, else I get the feeling it will stay there a very long time
> The information required for tests is print in YAML format. However, the output is not yet the same on all platforms. The current solution is to re-assign the encoding in a deterministic way. This code is responsible for the larger part of the increased compile time, and should be removed.
Could it be a separate diff?
I also share the worries about compilation time, it would be nice to have some more detailed measurement (ideally both on AMDGPU and AArch64) and maybe make some additional optimizations before this gets the green light.
(IMHO) Since this is a pretty big change I'd like to make sure it doesn't affect compilation times negatively, as a recurring theme in LLVM is slow compilation times and we don't want to make huge steps backwards.
================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelWorkList.h:115-118
+ bool contains(const MachineInstr *I) {
+ auto It = WorklistMap.find(I);
+ return It != WorklistMap.end();
+ }
----------------
================
Comment at: llvm/lib/CodeGen/GlobalISel/Combiner.cpp:102
+// WorkList ensures that the match sets are recalculated in the correct order.
+class TreeRepairer {
+ MachineRegisterInfo *MRI;
----------------
The class doesn't look like it repairs anything so the name could be better. If I understand correctly it builds a worklist from a list of changed instructions, adding the "parent" instructions as well. Maybe it should just be named `TreeWorklistBuilder` or something like that? I feel like it could also just be a pair of recursive function and the class itself adds no value.
Lastly, as this recursively adds all "parent" instructions I'm afraid it could "explode" in some cases and add almost all instructions. Is that possible?. Can you maybe look for optimizations/opportunities to skip some instructions? If there is none/they are rare, please add some comment explaining why we need to aggressively add everything
================
Comment at: llvm/lib/CodeGen/GlobalISel/Combiner.cpp:104
+ MachineRegisterInfo *MRI;
+ using WorkListTy = GISelWorkList<512>;
+ WorkListTy &WorkList;
----------------
nit: I would move typedefs to the top and not mix them with variable declarations for readability
================
Comment at: llvm/lib/CodeGen/GlobalISel/Combiner.cpp:125
+ for (MachineInstr *MI : ChangedInstrs) {
+ LLVM_DEBUG(dbgs() << "Adding path for: "; MI->print(dbgs()));
+ add(MI);
----------------
nit: I would split it in two lines
IMO debug statements like this are only useful with added context, so I would also print something before the loop runs, e.g. "Running TreeRepairer on N changed instructions". It would also be nice after each iteration to print how many instructions were added to the worklist. It could make it easier to spot cases where this "explodes" and greatly increases the number of instructions added to the worklist.
================
Comment at: llvm/lib/CodeGen/GlobalISel/Combiner.cpp:168-169
GISelWorkList<512> WorkList;
- WorkListMaintainer Observer(WorkList);
+ SmallVector<MachineInstr *, 32> ChangedInstrs;
+ WorkListMaintainer Observer(WorkList, ChangedInstrs);
GISelObserverWrapper WrapperObserver(&Observer);
----------------
If we're always passing a new set, can we just not pass it by reference to WorklistMaintainer and instead let it create it & use an acccessor to retrieve it later?
================
Comment at: llvm/utils/TableGen/GICombinerEmitter.cpp:731-734
+ for (unsigned VariantId : Info.getVariants()) {
+ OS.indent(Indent + 2) << "case " << VariantId << ":\n";
+ }
+
----------------
================
Comment at: llvm/utils/TableGen/GICombinerEmitter.cpp:742
+ // Emit the MIs[] array.
+ auto EmitMIs = [&OS](unsigned Indent, const GIMatchPatternInfo &Info) {
+ for (auto [Index, Value] : enumerate(Info.instr_infos())) {
----------------
================
Comment at: llvm/utils/TableGen/GICombinerEmitter.cpp:744-747
+ if (Index)
+ OS.indent(Indent) << "MIs[" << Index << "] = MRI.getVRegDef(MIs["
+ << Value.getBaseInstrID() << "]->getOperand("
+ << Value.getFromOpIdx() << ").getReg());\n";
----------------
It's one statement but more than one physical line so IIRC we want braces here
================
Comment at: llvm/utils/TableGen/GICombinerEmitter.cpp:757-758
+ EmitMIs(Indent + 8, Info);
+ for (auto I = Info.getVariants().begin(), E = Info.getVariants().end();
+ I != E;) {
+ unsigned VarId = *I;
----------------
small nit: I'm not sure I like for loops without the increment part, could this just be a while loop?
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:18
+///
+/// All patterns to match are known at compile. From these patterns, the match
+/// sets and the table lookups are constructed. The algorithm was first
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:26
+/// calculated, and so on.
+/// - Finally, the lookup tables con be constructed.
+///
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:33
+/// of of patterns, which can appear as subpatterns of a pattern. Using these
+/// representer sets, the calculation is speed up, and the tables are much
+/// smaller.
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:83
+
+using namespace llvm;
+
----------------
Unnecessary as all your code is inside `namespace llvm`
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:85
+
+/*
+ Some remarks about performance and memory usage
----------------
Are those TO-DO's to be done before this diff lands, or will they stay?
If they're here to stay, use `//` instead of `/* */`: https://llvm.org/docs/CodingStandards.html#comment-formatting
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:112
+ (c)
+ The implementation does not care about memory management. A context object
+ holding a bump allocator could be introduced to free most of the memory after
----------------
A bit scary to read and it would be useful to elaborate. Does it leak? Is it inefficient in terms of space (allocates too much) or time (e.g. chains of owned ptrs need to be freed and it takes time) ?
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:137
+// contains key-value pairs as DenseMap does.
+template <typename SetT, typename ElemT> class EnumSetRangeImpl {
+ const ArrayRef<SetT> &Dimension;
----------------
Could this (& its derived class) move to a separate header? (or just GIMatchTreeAutomaton.h)
This file is already pretty big and takes time to understand, having a whole datastructure definition in there doesn't help
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:157
+ // Initialize first element. Return if one of the sets is empty.
+ for (int I = 0, E = Dim; I < E; ++I) {
+ Iter[I] = Dimension[I].begin();
----------------
Why int here but unsigned in other places? (same above)
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:186
+ return *this;
+ }
+ ArrayRef<ElemT> operator*() const { return Elem; }
----------------
Ditto - newline after } and before next function
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:202
+ SmallVector<unsigned, 3> Elem;
+ int AtEnd;
+
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:261
+ SmallVector<unsigned, 3> Elem;
+ int AtEnd;
+
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:330-331
+ assert(Paren.size() == 2 && "Expected pair of parenthesis");
+ const char PO = Paren[0];
+ const char PC = Paren[1];
+ const unsigned LastDim = Dim[Dim.size() - 1];
----------------
nit: I would avoid 2 letter variable names like those. Maybe just use `OpenParen/CloseParen`?
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:644
+ // Lambda to retrieve the opcode predicate for an instruction.
+ auto getOpcodePredicate =
+ [&](const GIMatchDagInstr *Instr) -> const GIMatchDagOpcodePredicate * {
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:659
+ // - it adds edges in post-order.
+ std::function<unsigned(const GIMatchDagInstr *, GIMatchPatternInfo &,
+ unsigned, unsigned, unsigned, unsigned &)>
----------------
Typedef this to make it less verbose?
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:804-806
+ for (auto &MS : MatchSets) {
+ L.updateSA(MS.first);
+ }
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:813
+ // Compute all match sets.
+ for (bool Changes = true; Changes; ++H) {
+ Changes = false;
----------------
Hmm, this works but feels like a weird use for "for". Perhaps a while loop is better here?
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:821-823
+ for (auto P : EnumSet<BitVector, unsigned>(PatternSet)) {
+ updateMatchSetWithPattern(MS, L, P);
+ }
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:969
+void PatternForestBuilder::writeYAML(raw_ostream &OS) const {
+ auto WriteSet = [&OS](BitVector Set) {
+ OS << "[ " << Set << (Set.any() ? " " : "") << "]";
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:972
+ };
+ auto WriteSetOfSets = [&OS, &WriteSet](SetOfSetOfPatterns Sets,
+ unsigned Indent, bool EmitIdx) {
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:999-1001
+ for (const Pattern &P : Patterns) {
+ P.writeYAML(OS, 2);
+ }
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.cpp:1146-1148
+ for (auto &[Inst, MS] : LeafTables) {
+ OS << " " << Inst->TheDef->getName() << ": " << MS << "\n";
+ }
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.h:105
+
+ template <class Ty> Ty *getTargetData() const {
+ return static_cast<Ty *>(Data);
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.h:111
+ unsigned FromOpIdx) {
+ unsigned ID = addInstrInfo(BaseInstrID, FromOpIdx);
+
----------------
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.h:127
+ return ID;
+ }
+ size_t getNumInstrInfo() const { return InstrInfos.size(); }
----------------
small nit: Can you add a blank line between the } and the next function definitions to make it easier to read?
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.h:186
+
+ // LeafTables.
+ SmallVector<std::pair<const CodeGenInstruction *, unsigned>, 0> LeafTables;
----------------
nit: comment doesn't add anything, it just repeats the variable name. I would either remove it or elaborate.
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.h:189
+
+ // The matching rules.
+ DenseMap<unsigned, SmallVector<unsigned, 0>> MatchingRules;
----------------
Ditto
================
Comment at: llvm/utils/TableGen/GlobalISel/GIMatchTreeAutomaton.h:190
+ // The matching rules.
+ DenseMap<unsigned, SmallVector<unsigned, 0>> MatchingRules;
+ SmallVector<GIMatchPatternInfo, 0> MatchingRuleInfos;
----------------
If the key here is a number that starts at a fixed index and is always incremented by 1, you could just use a vector instead of a map
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141135/new/
https://reviews.llvm.org/D141135
More information about the llvm-commits
mailing list