The Pre-RA VLIWMachineScheduler used by Hexagon is a relatively generic implementation that would make sense to use on other VLIW targets. This commit lifts those classes into their own header/source file with the root VLIWMachineScheduler. I chose this path rather than adding the strategy et al. into MachineScheduler to avoid bloating the file with other implementations. Target-specific behaviors have been captured and replicated through function overloads. - Created an overloadable DFAPacketizer creation member function so that other targets are not restricted to the Packetizer created for the subtarget. - Added an extra helper which returns the number of instructions in the current packet. - Placed the priority heuristic values into the ConvergingVLIWscheduler class instead of them being local statics in the implementation - Added a overridable helper in ConvergingVLIWScheduler so that targets can create their own VLIWResourceModel
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This review is not yet formatted, but I'll upload a formatted version prior to commit once we find the changes sufficient. I wanted to avoid unrelated code changes from being injected into the review.
The actual changes are relatively painless. It's really all about what APIs we want to be overridable by inheritors. For now, I've only made virtual what needed to be made virtual for Hexagon. We should discuss others.
Comments for review added.
This has passed on my downstream, all-target Release build with expensive checks and assertions. The upstream base commit is 870fc844:"[ORC-RT] Add SPS serialization for span<const char> / SPSSequence<char>."
llvm/include/llvm/CodeGen/VLIWMachineScheduler.h | ||
---|---|---|
35 | Added a TargetInstrInfo so that targets have access to it in overrides | |
74 | API overrides:
| |
78 | This API is not currently called upstream, but is important for downstream, so I've added it. | |
82 | This function is redundant with the current design. However, downstream we've made changes to allow the concept of a "Packetizer", which has the API of DFAPacketizer, but without the requirement of being backed by the DFA created by tablegen. We then implement our own Packetizers, of which one is returned by CreateTargetScheduleState. Others may exist, however, and necessitate the ability to swap in custom implementations here. I'm not opposed to removing it for clarity's sake. | |
132 | Constants moved here to be static members instead of static locals in HexagonMachineScheduler.cpp I've noticed that static constexpr members, when utilized in LLVM, do not have backing definitions in source files. This is actually required by the C++ standard in case an expression like '&PriorityTwo' occurs. The later addition of consteval in C++20 removes the definition requirement, but since it seems commonplace to elide the definition in LLVM, I left it as-is. | |
261 | Helper to allow targets to inject their own VLIWResourceModel implementation | |
llvm/lib/CodeGen/MachineScheduler.cpp | ||
103–104 | It's very helpful to view dags, but the option to do this is locked behind a static variable. | |
llvm/lib/CodeGen/VLIWMachineScheduler.cpp | ||
76 | There is Hexagon-specific code here that has been lifted | |
264 | Uses of the VLIWResourceModel factory function | |
677 | There is Hexagon-specific code here that has been lifted |
FWIW: I like how this carefully extends the existing scheduler interfaces without affecting the non-VLIW cases!
I'll leave a detailed review to the users of this pass.
llvm/lib/CodeGen/VLIWMachineScheduler.cpp | ||
---|---|---|
10–11 | This seems copy&pasted from MachineScheduler.cpp and needs an update :) |
llvm/include/llvm/CodeGen/VLIWMachineScheduler.h | ||
---|---|---|
18 | Maybe some of the includes can be replaced with a class forward declaration like class RegisterClassInfo; in the header. | |
llvm/lib/CodeGen/VLIWMachineScheduler.cpp | ||
46 | Maybe the options could be prefixed with vliw- or similar... | |
105–108 | Just for my curiosity: Are you really seeing those instructions? I thought they are used in early parts of the codegen pipeline but should be lowered away after TwoAddressInstructionPass... |
llvm/include/llvm/CodeGen/VLIWMachineScheduler.h | ||
---|---|---|
46 | Correct: This is just what's upstream already. I'm more than happy to make changes and clean the objects up once we determine that this patch is good. The biggest gating issue right now is waiting to see if @kparzysz and team is able to apply and run their downstream Hexagon tests without seeing performance degradations. Once that's cleared, I will clang-format and make changes like this in earnest to get it cleared for commit. | |
llvm/lib/CodeGen/VLIWMachineScheduler.cpp | ||
46 | I was always hoping for some sort of option 'namespaces'. Does cl::opt allow colons? Perhaps something like vliwmisched:<option> to denote pass-specific options. | |
105–108 | I haven't done any investigation on my end. I don't think my downstream target utilizes them at the moment. This is, however, Hexagon's original implementation. |
I committed a small patch (https://reviews.llvm.org/rGa721ddbae983), it changes releaseTopNode and releaseBottomNode, you'll need to rebase before merging.
Performance tests look ok.
Thanks!
I appreciate you running the tests.
Next chance I get I'll rebase, clang-format, and apply the suggestions made here.
- Made ViewMISChedDAG a global option to match ForceTopDown et al.
- Optimized includes in VLIWMachineScheduler.h
- Replaced vectors in VLIWMachineScheduler.h with SmallVector
- Ran clang-format on all changes, which includes the entirety of the VLIWMachineScheduler files, as they are treated as 'new' files
Apologies. I uploaded the full diff again, rather than the refinement I wanted to.
The mechanics of the generalized pass have been accepted, but these changes exist to normalize the code to be more in line with the rest of the generic LLVM code base, as well as ran through clang-format.
llvm/include/llvm/CodeGen/MachineScheduler.h | ||
---|---|---|
104 | I really wasn't sure if this was the best way to hoist this option. Something similar is done in other files, but the NDEBUG switch between cl::opt<bool> and bool makes me a little bit uncomfortable. The other option is to instead put this block into VLIWMachineScheduler, but I feel that too has problems, since it's conceivable that any file which includes MachineScheduler may want access to these options. | |
llvm/include/llvm/CodeGen/VLIWMachineScheduler.h | ||
50 | A number of header implementations have been moved to the source file to allow the header to be more independent of header inclusion, This is an example. | |
llvm/lib/CodeGen/MachineScheduler.cpp | ||
94 | These ended up being moved into the llvm namespace. All but 1 example of options declared in the header were placed into the namespace, and doing so made intuitive sense to me so that the global namespace remains relatively clean. | |
llvm/lib/CodeGen/VLIWMachineScheduler.cpp | ||
45 | I forgot this comment... I'll prefix generic-looking options with vliw-misched, since that is a natural prefix |
I really wasn't sure if this was the best way to hoist this option. Something similar is done in other files, but the NDEBUG switch between cl::opt<bool> and bool makes me a little bit uncomfortable.
The other option is to instead put this block into VLIWMachineScheduler, but I feel that too has problems, since it's conceivable that any file which includes MachineScheduler may want access to these options.