Page MenuHomePhabricator

[WIP] MachineFunction Properties
ClosedPublic

Authored by dschuff on Mar 23 2016, 5:22 PM.

Details

Summary

This is a strawman design for a set of "properties" that a
MachineFunction can have at particular points in time. Currently we have
MachineRegisterInfo::isSSA() and MachineRegisterInfo::tracksLiveness()
and we are also discussing the "post-RA" property (i.e. there are no
virtual register operands). We want an easy way for MachineFunctions to
declare that they require a particular property to hold, and that they
set or clear a particular property. This facility is somewhat similar to
the way passes can declare their required analysis passes, inviting a
comparison of the requirements/use cases:

  1. Like analysis passes, passes can require that property hold.
  2. Unlike analysis, all possible properties can be listed in one place as an enum (as opposed to an arbitrary pass ID)
  3. Properties can be set by a pass (unlike analysis where the anlysis is provided by a special kind of pass and managed by a pass manager)
  4. Properties may be conditionally cleared (e.g. BranchFolder conditionally invalidates liveness)

This design just represents the properties as a bit vector stored on the
MachineFunctionObject, and the pass requirements and modifications as
objects on the MachineFunctionPass. Passes can declare that they
require, or unconditionally set or clear properties by setting these
objects in their constructor, (which results in automatic checking) or
they can query or modify the MachineFunction's properties when the pass
is run.

Currently it just implements the "post-RA" property as
AllVRegsAllocated, for X86. It still needs a few fixes and name bikeshedding.

Possible alternative design features/constraints:

  • Allow passes to require that a property is clear (instead of just set)
  • Make all passes require a particular property unless they clear the requirement themselves
  • Allow targets to extend the list

Possible alternative implementations:

  • Return set of set/cleared properties from a query function (similar to getAnalysisUsage) instead of keeping a static data structure set in the constructor

Diff Detail

Repository
rL LLVM

Event Timeline

dschuff updated this revision to Diff 51498.Mar 23 2016, 5:22 PM
dschuff retitled this revision from to [WIP] MachineFunction Properties.
dschuff updated this object.
dschuff added reviewers: qcolombet, hfinkel.
dschuff added a subscriber: llvm-commits.
dschuff updated this object.Mar 24 2016, 9:24 AM
dschuff updated this object.
dschuff updated this object.
qcolombet edited edge metadata.Mar 24 2016, 11:21 AM

Hi Derek,

I like the direction of your patch. I would go one step further and have the pass manager checks that required properties are set before running a pass (and error out if this is false).

I could see value in having the properties extendable by the target, in particular if the pass manager checks the dependencies for us.

Couple of comments inlined.

Thanks,
-Quentin

include/llvm/CodeGen/MachineFunction.h
95 ↗(On Diff #51498)

I think it would be nice to have this extendable by the target, especially if the properties are checked and/or give some control over the compilation pipeline.
E.g., On ARM, passes could set a property has load/store and the load/store optimizer would be only run if such property is true. The example is a bit lame, but you get the idea.

Obviously, this can already be achieved by querying within the passes the MachineFunctionInfo instances, but it sounds reasonable to have this kind of broad mechanism.

108 ↗(On Diff #51498)

I think this does not follow the coding standard:
Enumerators (e.g. enum { Foo, Bar }) [...] should start with an upper-case letter, just like types.

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

include/llvm/CodeGen/MachineFunctionPass.h
56 ↗(On Diff #51498)

We may allow to return nullptr. Not all passes have properties.
Another other would be to have a static const empty set MachineFunctionProperties, used when those pointers are null.

65 ↗(On Diff #51498)

If you go that way, I would make them lazily allocated.
I.e., mutable should be in order :).

lib/CodeGen/FuncletLayout.cpp
29 ↗(On Diff #51498)

My 2c; I would find it more natural to define this kind of dependencies in something like getAnalysisUsage, possibly something new.

The value, I see out of this, is the pass manager could check this kind of dependencies for us and error out is something is not correct.

lib/Target/NVPTX/NVPTXTargetMachine.cpp
229 ↗(On Diff #51498)

This is an unrelated change, right?

lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
110 ↗(On Diff #51498)

Ditto.

186 ↗(On Diff #51498)

Ditto.

214 ↗(On Diff #51498)

Ditto.

222 ↗(On Diff #51498)

Ditto.

tstellarAMD added inline comments.
include/llvm/CodeGen/MachineFunction.h
107 ↗(On Diff #51498)

You may want to clarify what kAllVRegsAllocated means, because targets are allowed to introduce new virtual registers after register allocation in TargetInstrInfo::eliminateFrameIndex(). These are eliminated by PEI using the register scavenger.

dschuff updated this revision to Diff 51685.Mar 25 2016, 2:26 PM
dschuff marked 5 inline comments as done.
dschuff edited edge metadata.
  • Change interface to be more like getAnalysisUsage
include/llvm/CodeGen/MachineFunction.h
95 ↗(On Diff #51498)

This kind of use does seem reasonable; that kind of thing would have the caveat that in order to use it, any pass or function that added load or store instructions would have to remember to set the property. I'll have to think a bit more about what the extension mechanism would look like. A lot of the simplicity of this current form is owed to the fact that all of the bits are specified in one place.

107 ↗(On Diff #51498)

Yes; although notably this all happens within the run of the PEI pass. So while it has a transient state, it does preserve the invariant.
But yes, the exact meaning of these should be clarified, and we should probably revisit the naming of things.

include/llvm/CodeGen/MachineFunctionPass.h
56 ↗(On Diff #51498)

(addressed with a diferent interface style; see below)

65 ↗(On Diff #51498)

(ditto)

lib/CodeGen/FuncletLayout.cpp
29 ↗(On Diff #51498)

I originally did it this way to avoid having to call getRequiredProperties (and thus construct the required, set, and cleared properties on every function). However I found another way around that, so I change it to be more like getAnalysisUsage, which I agree is nicer (and now has the added benefit that e.g. the required properties can't accidentally get modified during runtime).

Because these properties only apply to MachineFunctionPasses, and because Pass managers only deal with FunctionPasses, I still think it's cleaner to do the checking in MachineFunctionPass::runOnFunction() instead of propagating all of this stuff out to the pass manager via public interfaces. Note that as it is right now, it still has the nice property that the dependencies are automatically checked for us (where "us" is the authors of machine function passes).

lib/Target/NVPTX/NVPTXTargetMachine.cpp
229 ↗(On Diff #51498)

Actually, it's not; these are all passes that are running today in NVPTX and WebAssembly that were originally written as post-RA passes, but have no particular assertions or dependencies on the lack of virtual registers, and so were not explicitly disabled. (They mostly do nothing for virtual architectures). But since I've now added this requiredProperty mechanism, they now have to be disabled, or else they will fail the check.

So basically this is exactly the effect that you wanted all along; these passes can't be run for virtual targets now (until and unless someone ensures that they support virtual registers and changes the requirement).

qcolombet accepted this revision.Mar 25 2016, 4:33 PM
qcolombet edited edge metadata.

Hi Derek,

Simple and efficiency, nice :).

LGTM.

I agree that, as it is, the checks are enough, we do not need to expose this to the pass manager.

Couple of nits inlined.

Cheers,
-Quentin

include/llvm/CodeGen/MachineFunction.h
95 ↗(On Diff #51685)

“Promote” the comment to doxygen style (///).

98 ↗(On Diff #51685)

Just a remark: if we want to give more control to the targets with possible properties, we can make the universe of the BitVector an argument of the constructor and just assert that the universe supports at least LastProperty elements.

210 ↗(On Diff #51685)

Add a comment.

lib/CodeGen/MachineFunctionPass.cpp
50 ↗(On Diff #51685)

Add a message in the assert.

lib/Target/NVPTX/NVPTXTargetMachine.cpp
229 ↗(On Diff #51685)

Great!
It means it already pays off by simplifying some compilation pipelines :).

This revision is now accepted and ready to land.Mar 25 2016, 4:33 PM
dschuff updated this revision to Diff 51704.Mar 25 2016, 5:31 PM
dschuff marked 3 inline comments as done.
dschuff edited edge metadata.
  • Change interface to be more like getAnalysisUsage
  • Address comments
  • Add AllVRegsAllocated to MIR and convert one test

Thanks for the reviews!
There's one more thing this change needs before it can land, and that's a representation in MIR for the properties. Conveniently, my reviewer is an expert on the subject :)

Here's one possible MIR representation, which just adds allVRegsAllocated as another optional top-level tag for the function for each property (and which would also retain compatibility with MIR for e.g. isSSA). Do you think it should also have its own namespace like frameInfo, or maybe its own initialization routine like MIRParserImpl::initializeRegisterInfo?

include/llvm/CodeGen/MachineFunction.h
98 ↗(On Diff #51685)

Sounds good, I added possible future TODOs in a comment.

dschuff updated this revision to Diff 51801.Mar 28 2016, 9:56 AM
  • Change interface to be more like getAnalysisUsage
  • Address comments
  • Add AllVRegsAllocated to MIR and convert one test
  • Update the remaining MIR tests
This revision was automatically updated to reflect the committed changes.