This is an archive of the discontinued LLVM Phabricator instance.

[PM] Port SROA to the new pass manager.
ClosedPublic

Authored by chandlerc on Sep 10 2015, 2:02 PM.

Details

Summary

In some ways this is a very boring port to the new pass manager as there
are no interesting analyses or dependencies or other oddities.

However, this does introduce the first good example of a transformation
pass with non-trivial state porting to the new pass manager. I've tried
to carve out patterns here to replicate elsewhere, and would appreciate
comments on whether folks like these patterns:

  • A common need in the new pass manager is to effectively lift the pass class and some of its state into a public header file. Prior to this, LLVM used anonymous namespaces to provide "module private" types and utilities, but that doesn't scale to cases where a public header file is needed and the new pass manager will exacerbate that. The pattern I've adopted here is to use the namespace-cased-name of the core pass (what would be a module if we had them) as a module-private namespace. Then utility and other code can be declared and defined in this namespace. At some point in the future, we could even have (conditionally compiled) code that used modules features when available to do the same basic thing.
  • I've split the actual pass run method in two in order to expose a private method usable by the old pass manager to wrap the new class with a minimum of duplicated code. I actually looked at a bunch of ways to automate or generate these, but they are all quite terrible IMO. The fundamental need is to extract the set of analyses which need to cross this interface boundary, and that will end up being too unpredictable to effectively encapsulate IMO. This is also a relatively small amount of boiler plate that will live a relatively short time, so I'm not too worried about the fact that it is boiler plate.

The rest of the patch is totally boring but results in a massive diff
(sorry). It just moves code around and removes or adds qualifiers to
reflect the new name and nesting structure.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc updated this revision to Diff 34487.Sep 10 2015, 2:02 PM
chandlerc retitled this revision from to [PM] Port SROA to the new pass manager..
chandlerc updated this object.
chandlerc added reviewers: dexonsmith, bogner.
chandlerc added a subscriber: llvm-commits.
silvas added a subscriber: silvas.Sep 10 2015, 3:26 PM

In [LLVMdev] Heads up: Pass Manager changes will be starting shortly one
of the goals was "I'm going to build a bridge so that an existing Pass can
be inserted into the new pass manager with some adaptors and everything
will just work".

My reading of this is that there would be an incremental step where we have
fully transitioned to using the new pass manager, but existing passes are
using wrappers. We would then incrementally move each pass to "natively"
use the new pass manager as needed (e.g. for the inliner). So I'm surprised
that we are porting SROA so early.

Did I miss/forget something? I've noticed that in last fall's devmtg talk
it looked like you have "port the existing pass pipeline" as a blocking
item, so I assume there was some sort of realization in the interim. Could
you elaborate on this (or point to a thread I missed?).

  • Sean Silva
chandlerc updated this revision to Diff 34529.Sep 10 2015, 9:26 PM

Rebase and add the last missing bits of the port, including a test of it with
the new PM.

Should be in a good state to land at this point.

mehdi_amini added inline comments.
include/llvm/Transforms/Scalar/SROA.h
11 ↗(On Diff #34529)

s/pimary/primary/

sanjoy added a subscriber: sanjoy.Sep 10 2015, 11:36 PM
sanjoy added inline comments.
lib/Transforms/Scalar/SROA.cpp
4275 ↗(On Diff #34529)

Should this be return !PA.areAllPreserved();?

sanjoy added inline comments.Sep 10 2015, 11:40 PM
lib/Transforms/Scalar/SROA.cpp
4202 ↗(On Diff #34529)

This is super minor, but do you need to pass the context separately? Is it ever different from F.getContext()?

chandlerc updated this revision to Diff 34615.Sep 11 2015, 7:05 PM

Fix bug spotted by Sanjoy, and make the improvements suggested by him and
Justin.

This revision was automatically updated to reflect the committed changes.