This is an archive of the discontinued LLVM Phabricator instance.

[LegacyPassManager] Remove TargetMachine constructors
ClosedPublic

Authored by thegameg on May 15 2017, 5:25 PM.

Details

Summary

[LegacyPassManager] Remove TargetMachine constructors

This provides a new way to access the TargetMachine through TargetPassConfig, as a dependency.

The patterns replaced here are:

  • Passes handling a null TargetMachine call getAnalysisIfAvailable<TargetPassConfig>.
  • Passes not handling a null TargetMachine addRequired<TargetPassConfig> and call getAnalysis<TargetPassConfig>.
  • MachineFunctionPasses now use MF.getTarget().
  • Remove all the TargetMachine constructors. Remove INITIALIZE_TM_PASS.

This fixes a crash when running llc -start-before prologepilog.

PEI needs StackProtector, which gets constructed without a TargetMachine by the pass manager. The StackProtector pass doesn't handle the case where there is no TargetMachine, so it segfaults.

Related to PR30324.

Diff Detail

Event Timeline

thegameg created this revision.May 15 2017, 5:25 PM
thegameg edited the summary of this revision. (Show Details)May 15 2017, 5:27 PM
MatzeB edited edge metadata.

Thanks, this looks really interesting. Adding some more reviewers who may be interested in pass manager and codegen things.

Just wanted to point to https://reviews.llvm.org/D29494, which is a patch that isn't fully polished to split the StackProtector pass into an Analysis pass and a Transformation pass.
My motivation for that was also fixing PR30324.

chandlerc edited edge metadata.May 16 2017, 9:06 AM

Cool! This seems to me like a really nice simplification given the fact that the LPM has significant constraints on how you construct passes.

lib/CodeGen/StackProtector.cpp
101โ€“104

I would simple unconditionally assign here like we do with other things.

lib/Target/Mips/MipsModuleISelDAGToDAG.cpp
46โ€“47

This is horrible. =/

I think the MipsTargetMachine should directly expose a const-qualified resetSubtarget method, as it is already doing crazy const-casting... Or it should have some other way of doing this.

I might just leave the original code for this part of the Mips backend rather than trying to make it clean in the same way.

MatzeB accepted this revision.May 16 2017, 1:00 PM

Thanks, this turned out to be a nice code simplification!
LGTM with nitpicks (including Chandlers comments) addressed.

lib/CodeGen/TargetPassConfig.cpp
318

I assume this is the message people will see when they schedule a codegen pass without a target machine now. Could you maybe improve the error message to be more helpful?
"Trying to construct TargetPassConfig without target machine. Scheduling a CodeGen pass without a target triple set?"

lib/Target/Mips/MipsModuleISelDAGToDAG.cpp
46โ€“47

I assume you can address this by getting a non-const TargetMachine from the TargetPassConfig.

lib/Target/Sparc/LeonPasses.h
34โ€“35

It looks like this constructor can be removed completely now. Could you try?

This revision is now accepted and ready to land.May 16 2017, 1:00 PM
thegameg updated this revision to Diff 99191.May 16 2017, 1:52 PM
thegameg marked 5 inline comments as done.
  • Changed the error message when constructing a TPC without a target machine. Replaced llvm_unreachable with report_fatal_error.
  • Added test to check for the error.
  • Updated MipsModuleDAGToDAGISel to use TPC instead of MF.getTarget(). This avoids const_cast craziness.
  • Removed the LeonMachineFunctionPass default constructor.
echristo accepted this revision.May 17 2017, 3:59 PM

This LGTM. I'd get one from Chandler as well though.

lib/Target/Mips/MipsModuleISelDAGToDAG.cpp
46

Formatting nit.

46โ€“47

The mips target just needs to remove their subtarget like arm, aarch64, ppc, and x86.

This LGTM. I'd get one from Chandler as well though.

I intended my prior comment to indicate it LGTM with the changes requested.

This revision was automatically updated to reflect the committed changes.
thegameg marked an inline comment as done.