Page MenuHomePhabricator

Lift VLIWResourceModel, VLIWMachineScheduler, and ConvergingVLIWScheduler into CodeGen/VLIWMachineScheduler
ClosedPublic

Authored by JamesNagurne on Nov 3 2021, 3:37 PM.

Details

Summary
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

Diff Detail

Event Timeline

JamesNagurne created this revision.Nov 3 2021, 3:37 PM
JamesNagurne requested review of this revision.Nov 3 2021, 3:37 PM

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
34

Added a TargetInstrInfo so that targets have access to it in overrides

73

API overrides:

  • Check if two SUnits conflict
  • Check if SUnit can be added to current state
  • Actually add item to current state
77

This API is not currently called upstream, but is important for downstream, so I've added it.

81

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.

131

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.

260

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.
I've seen elsewhere that using a non-static cl::opt and an extern declaration elsewhere is a valid way to make the option available in other source, and applied that here.

llvm/lib/CodeGen/VLIWMachineScheduler.cpp
75

There is Hexagon-specific code here that has been lifted

263

Uses of the VLIWResourceModel factory function

676

There is Hexagon-specific code here that has been lifted

MatzeB added a comment.Nov 3 2021, 4:41 PM

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
9–10

This seems copy&pasted from MachineScheduler.cpp and needs an update :)

MatzeB added inline comments.Nov 3 2021, 4:53 PM
llvm/include/llvm/CodeGen/VLIWMachineScheduler.h
17

Maybe some of the includes can be replaced with a class forward declaration like class RegisterClassInfo; in the header.

llvm/lib/CodeGen/VLIWMachineScheduler.cpp
45

Maybe the options could be prefixed with vliw- or similar...

104–107

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...

MatzeB added inline comments.Nov 3 2021, 4:56 PM
llvm/include/llvm/CodeGen/VLIWMachineScheduler.h
34–41

Consider using references instead of pointers for things that cannot change and cannot be nullptr.

45

Isn't SmallVector<> typically the better choice over std::vector?

JamesNagurne added inline comments.Nov 4 2021, 11:24 AM
llvm/include/llvm/CodeGen/VLIWMachineScheduler.h
45

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
45

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.

104–107

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.

kparzysz accepted this revision.Nov 9 2021, 9:47 AM

LGTM

This revision is now accepted and ready to land.Nov 9 2021, 9:47 AM

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 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.

JamesNagurne added inline comments.Nov 17 2021, 1:50 PM
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
49–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–46

I forgot this comment...

I'll prefix generic-looking options with vliw-misched, since that is a natural prefix

Updating diff to most recent: Want to see if CI actually starts working..