This is an archive of the discontinued LLVM Phabricator instance.

Require MachineFunctionPasses to declare their support for virtual registers.
AbandonedPublic

Authored by dschuff on Jan 22 2016, 12:27 PM.

Details

Reviewers
qcolombet
Summary

Not ready for commit yet; only includes X86 and WebAssembly.
Adds a method to MachineFunctionPass for derived classes to declare that they
support virtual registers, and an assert in the base runOnFunction to
enforce it. The idea is to make it more clear which passes support virtual
registers and to avoid accidentally exposing VRegs to passes which aren't
prepared to handle them.

Diff Detail

Event Timeline

dschuff updated this revision to Diff 45728.Jan 22 2016, 12:27 PM
dschuff retitled this revision from to Require MachineFunctionPasses to declare their support for virtual registers..
dschuff updated this object.
dschuff added a reviewer: qcolombet.
dschuff added a subscriber: llvm-commits.

Ping. any comments on the general approach or specific mechanism used here?

dschuff added a subscriber: hfinkel.Feb 4 2016, 1:45 PM

+@hfinkel because of his comment on D16481 and the mailing list

hfinkel added inline comments.Feb 4 2016, 3:35 PM
include/llvm/CodeGen/MachineFunctionPass.h
61

I think that what you gain in brevity you lose in clarity, because while the macro is shorter than the function call, it is now not immediately obvious what it does. Just include the function override.

An alternative is to make it an argument to the MachineFunctionPass constructor.

Also, I'm not entirely sure this is a problem worth solving, at least not as a one-off. We already have passes that only work either before or after register allocation, in or not in SSA form, before or after PHI elimination, etc. -- and we don't explicitly declare any of that.

Generically, it would be nice to express these dependencies in a more-consistent way, and I'm in favor of something that let's us do that. We already have a facility in the pass manager to express requirements, maybe we can build on that?

dschuff added inline comments.Feb 4 2016, 4:03 PM
include/llvm/CodeGen/MachineFunctionPass.h
61

Yeah it was my intention to expand the macros manually (and fill in the rest of the architectures) before committing. I guess I mentioned that in the mailing list post but not here.

As for general requirements, that's not a bad idea either. I toyed with making this a sort of pseudo analysis, but if we made some other notion about properties of the MIR at a particular time (is SSA, has virtual registers, etc) that would fit.

qcolombet edited edge metadata.Feb 5 2016, 8:49 AM

Hi Derek,

I’m catching up with emails now.
I think I’ll get to this (and your other thread) next week.

Cheers,
-Quentin

qcolombet added inline comments.Feb 10 2016, 4:20 PM
include/llvm/CodeGen/MachineFunctionPass.h
61

I toyed with making this a sort of pseudo analysis, but if we made some other notion about properties of the MIR at a particular time (is SSA, has virtual registers, etc) that would fit.

I didn’t get that. I am guessing you wanted to say “wouldn’t fit” since the pass manager has currently no way to tell what happened at a particular time.

Anyhow, for the problem we are trying to solve, I am not sure a single boolean is enough. For instance, the peephole optimizer supports virtual registers, but only works on SSA code. I agree with Hal that what we want is a way to express dependencies in a more-consistent way.

To get concrete here, I think that we could start with some properties on the MachineFunction (like isSSA). Then, the machine verifier would check the assumptions are correct based on them, like after register allocation we don’t have any virtual registers anymore. Other passes would have a way to express what they support based on those properties. Finally, if we try to schedule a pass whereas the properties do not match what it supports, this is an error.

I investigated a more generic mechanism and its requirements in D18421, have a look.

dschuff abandoned this revision.Apr 4 2016, 12:58 PM

Superseded by D18421