Page MenuHomePhabricator

[SimplifyCFG] use pass options and remove the latesimplifycfg pass
ClosedPublic

Authored by spatel on Oct 6 2017, 9:24 AM.

Details

Summary

This is no-functional-change-intended, but there's enough churn here that I thought it would be best to do this as a preliminary patch for D38566.

This is repackaging the functionality of D30333 (defer switch-to-lookup-tables) and D35411 (defer folding unconditional branches) with pass parameters rather than a named "latesimplifycfg" pass. Now that we have individual options to control the functionality, we could decouple when these fire (but that's an independent patch if desired). The next planned step would be to add another option bit to disable the sinking transform mentioned in D38566.

As mentioned in D38138, I don't know if it's intentional or oversight that we don't have a "latesimplifycfg" equivalent invocation with the new pass manager pipeline. To make that explicit, I removed the default constructor used by that PM, so we'd be aware that we're always creating an "early" (ie, not fully functional) version of the simplifycfg pass.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Oct 6 2017, 9:24 AM
spatel planned changes to this revision.Oct 22 2017, 11:15 AM
spatel added a reviewer: dberlin.

My description of the new pass manager behavior was wrong and inverted. It doesn't always create "-earlysimplifycfg" equivalent invocations. It always creates "-latesimplifycfg" equivalent invocations. I need to correct this patch to preserve that behavior (and add tests so it isn't accidentally broken).

It's also likely that we'll have another options bit in the near-term to correct the mistakes pointed out in D39011.

spatel updated this revision to Diff 119899.Oct 23 2017, 10:49 AM

Patch updated:

  1. We have another option intended to limit simplifycfg around switches early in the pipeline with rL316298.
  2. Fixed the new pass manager code to continue to create latesimplifycfg-equivalent passes because this patch is NFCI. I created a phase ordering test for both PMs (it's based on the example from D35411) to show the bug and added it with rL316351.
hfinkel edited edge metadata.Oct 25 2017, 6:45 PM

Generally, I think this is the right thing to do. I don't like the API, however. We have SimplifyCFGOptions, which is good, but here essentially all users pass something like {1, false, false, true}, which defeats the point because I'll never remember which boolean is which. Maybe we could use a "builder API" for this, something like

SimplifyCFGOptions().enableForwardSwitchCondToPhi().enableConvertSwitchToLookupTable().disableNeedCanonicalLoop()

would be better (so that we can have names instead of just booleans and numbers)?

include/llvm/Transforms/Scalar/SimplifyCFG.h
35 ↗(On Diff #119899)

This seems like overkill. We do have a current set of defaults (whatever we use for canonicalization), and defaulting to canonicalization seems reasonable to me. We can change the defaults if we change what our canonical form is.

spatel updated this revision to Diff 120589.Oct 27 2017, 7:03 AM

Patch updated:

  1. Keep default SimplifyCFGPass constructor and use it where appropriate (reminder: this patch is NFCI, so I preserved the non-default instantiations in the new PM with one FIXME comment).
  2. Add methods to SimplifyCFGOptions to allow builder constructor pattern which provide easier-to-read instantiations of SimplifyCFGPass (please make sure I did that correctly because I've never tried that before!).
bmakam added inline comments.Oct 27 2017, 7:48 AM
include/llvm/Transforms/Scalar.h
263 ↗(On Diff #120589)

We still pass boolean and values to createCFGSimplificationPass(1, false, false, true) Don't we need builder API here as well?

lib/Transforms/IPO/PassManagerBuilder.cpp
630 ↗(On Diff #120589)

We still pass booleans and values here. See my comment above for builder API.

spatel added inline comments.Oct 27 2017, 8:04 AM
include/llvm/Transforms/Scalar.h
263 ↗(On Diff #120589)

This is only used by the legacy PM unless I'm not seeing it correctly. Fixing this will require more futzing, possibly adding includes for the header or moving the struct definition from its current location. So I was hoping that we'd let that go given that the new PM seems relatively close to becoming default.

spatel added inline comments.Oct 27 2017, 8:09 AM
include/llvm/Transforms/Scalar.h
185–187 ↗(On Diff #120589)

This is an example of the legacy PM API that I modeled the simplifyCFG creator wrapper on.

Patch updated:

  1. Keep default SimplifyCFGPass constructor and use it where appropriate (reminder: this patch is NFCI, so I preserved the non-default instantiations in the new PM with one FIXME comment).

Okay. I understand that you want to switch the defaults in a follow-up patch. Why, however, don't you make SimplifyCFGOptions() .forwardSwitchCondToPhi(true).convertSwitchToLookupTable(true).needCanonicalLoops(false) the default, and then change the default in a follow-up patch?

  1. Add methods to SimplifyCFGOptions to allow builder constructor pattern which provide easier-to-read instantiations of SimplifyCFGPass (please make sure I did that correctly because I've never tried that before!).
include/llvm/Transforms/Scalar.h
263 ↗(On Diff #120589)

Okay (although this is slightly suboptimal for the backends which I imagine will use the legacy pass manager longer).

spatel updated this revision to Diff 120693.Oct 27 2017, 2:15 PM

Patch updated:
Use the "late" options in the default SimplifyCFGPass constructor. This makes the next (functional) patch smaller and eliminates all of the diffs from lib/Passes/PassBuilder.cpp in this one.

hfinkel accepted this revision.Oct 27 2017, 4:57 PM

Patch updated:
Use the "late" options in the default SimplifyCFGPass constructor. This makes the next (functional) patch smaller and eliminates all of the diffs from lib/Passes/PassBuilder.cpp in this one.

LGTM

This revision is now accepted and ready to land.Oct 27 2017, 4:57 PM
This revision was automatically updated to reflect the committed changes.