This is an archive of the discontinued LLVM Phabricator instance.

[NFC][RFC][TableGen] Split GlobalISelEmitter.cpp
ClosedPublic

Authored by Pierre-vh on May 25 2023, 5:21 AM.

Details

Summary

This patch splits the GlobalISelEmitter.cpp file, which imports DAG ISel patterns for GISel, into separate "GISelMatchTable.h/cpp" files.

The main motive is readability & maintainability. GlobalISelEmitter.cpp was about 6400 lines of mixed code, some bits implementing the match table codegen, some others dedicated to importing DAG patterns.

Now it's down to 2700 + a 2150 header + 2000 impl.
It's a tiny bit more lines overall but that's to be expected - moving
inline definitions to out-of-line, adding comments in the .cpp, etc. all of that takes additional space, but I think the tradeoff is worth it.

I did as little unrelated code changes as possible, I would say the biggest change is the introduction of the gi namespace used to prevent name conflicts/ODR violations with type common names such as Matcher.
It was previously not an issue because all of the code was in an anonymous namespace.

This moves all of the "match table" code out of the file, so predicates,
rules, and actions are all separated now. I believe this helps separating concerns, now GlobalISelEmitter.cpp is more focused on importing DAG patterns into GI, instead of also containing the whole match table internals as well.

Note: the new files have a "GISel" prefix to make them distinct from the other "GI" files in the same folder, which are for the combiner.

Diff Detail

Event Timeline

Pierre-vh created this revision.May 25 2023, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 5:21 AM
Pierre-vh requested review of this revision.May 25 2023, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 5:21 AM
aemerson accepted this revision.Jun 1 2023, 3:15 PM

I think this is a good refactor, thanks for doing it.

This revision is now accepted and ready to land.Jun 1 2023, 3:15 PM
This revision was automatically updated to reflect the committed changes.
Pierre-vh reopened this revision.Jun 5 2023, 12:41 AM

Had to revert, this is causing build failures (that I didn't see locally) because of undefined symbols:

mold: error: undefined symbol: llvm::CodeGenTarget::getRegNamespace() const
>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::AddRegisterRenderer::emitRenderOpcodes(llvm::gi::MatchTable&, llvm::gi::RuleMatcher&) const)
mold: error: undefined symbol: llvm::CodeGenTarget::ComputeInstrsByEnum() const
>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::InstructionOpcodeMatcher::initOpcodeValuesMap(llvm::CodeGenTarget const&))
mold: error: undefined symbol: llvm::TreePredicateFn::getImmTypeIdentifier() const
>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::(anonymous namespace)::getEnumNameForPredicate[abi:cxx11](llvm::TreePredicateFn const&))>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::InstructionImmPredicateMatcher::emitPredicateOpcodes(llvm::gi::MatchTable&, llvm::gi::RuleMatcher&) const)
mold: error: undefined symbol: llvm::TypeSetByHwMode::getValueTypeByHwMode() const
>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::OperandMatcher::addTypeCheckPredicate(llvm::TypeSetByHwMode const&, bool))>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::OperandMatcher::addTypeCheckPredicate(llvm::TypeSetByHwMode const&, bool))>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::OperandMatcher::addTypeCheckPredicate(llvm::TypeSetByHwMode const&, bool))
mold: error: undefined symbol: llvm::TreePredicateFn::hasGISelPredicateCode() const
>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::(anonymous namespace)::getEnumNameForPredicate[abi:cxx11](llvm::TreePredicateFn const&))
mold: error: undefined symbol: llvm::TreePredicateFn::getFnName[abi:cxx11]() const
>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::(anonymous namespace)::getEnumNameForPredicate[abi:cxx11](llvm::TreePredicateFn const&))>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::(anonymous namespace)::getEnumNameForPredicate[abi:cxx11](llvm::TreePredicateFn const&))
mold: error: undefined symbol: llvm::CodeGenRegisterClass::getQualifiedName[abi:cxx11]() const
>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::RegisterBankOperandMatcher::emitPredicateOpcodes(llvm::gi::MatchTable&, llvm::gi::RuleMatcher&) const)>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::ConstrainOperandToRegClassAction::emitActionOpcodes(llvm::gi::MatchTable&, llvm::gi::RuleMatcher&) const)
mold: error: undefined symbol: llvm::CodeGenSubRegIndex::getQualifiedName[abi:cxx11]() const
>>> referenced by GISelMatchTable.cpp
>>>               utils/TableGen/GlobalISel/CMakeFiles/obj.LLVMTableGenGlobalISel.dir/GISelMatchTable.cpp.o:(llvm::gi::TempRegRenderer::emitRenderOpcodes(llvm::gi::MatchTable&, llvm::gi::RuleMatcher&) const)

It seems to be because the "TableGen/GlobalISel" folder is a separate library, for some reason?

This revision is now accepted and ready to land.Jun 5 2023, 12:41 AM
Pierre-vh updated this revision to Diff 528305.Jun 5 2023, 12:53 AM

(Hopefully) fix the linker errors

Despite a full clear of my build dir they don't appear for me. I think the buildbot was using a different linker.
In any case, this should be solved by moving the files to the same library that contains the GlobalISelEmitter.cpp

Pierre-vh requested review of this revision.Jun 5 2023, 6:24 AM

No big changes other than moving the files to the parent library to avoid linker issues.
I don't really see why TableGen/GlobalISel needs to be a separate library when it looks like it's just utils for the GI TableGen backend but I guess it's a discussion for another day

aemerson accepted this revision.Jun 6 2023, 10:26 AM
This revision is now accepted and ready to land.Jun 6 2023, 10:26 AM
This revision was automatically updated to reflect the committed changes.