This is an archive of the discontinued LLVM Phabricator instance.

Add flag to PassManagerBuilder to disable GVN Hoist Pass.
ClosedPublic

Authored by asbirlea on Jul 21 2016, 11:35 AM.

Diff Detail

Event Timeline

asbirlea updated this revision to Diff 64934.Jul 21 2016, 11:35 AM
asbirlea retitled this revision from to Add flag to PassManagerBuilder to disable GVN Hoist Pass..
asbirlea updated this object.
davide added a subscriber: davide.Jul 21 2016, 11:45 AM

You might want to add a cl::opt to control the behavior as load-combine does, maybe?
See in IPO/PassManagerBuilder.cpp

static cl::opt<bool> RunLoadCombine("combine-loads", cl::init(false),
                                    cl::Hidden,
                                    cl::desc("Run the load combining pass"));

Halide uses the builder and not opt, so it wasn't a requirement in our use case, but it may be helpful for others. I'll update shortly, thanks!

asbirlea updated this revision to Diff 64937.Jul 21 2016, 12:01 PM

Add opt flag.

These kind of targetted flags on the PassManagerBuilder are really ugly (and there are too many), we should avoid this as much as possible. Can the faulty Halide test be disabled instead?

Let me rephrase, I had an inappropriate use of "ugly" here:

I'm not fond of these kind of targetted flags (on the contrary to "optimization level" for instance) on the PassManagerBuilder. The current state is not really satisfactory (too many, and they are not documented), and it shows that we don't have a principled solution to control this.
It could slip toward a DisableXXX for each pass ultimately.

Since the motivation is "unblock temporarily the bots during the investigation of the bug" Can the faulty Halide test be disabled instead?

The problem is not a Halide test but a scalability issue in MemorySSA that's making Halide unusable, consuming gigabytes of memory and taking forever. Personally, I don't think the GVNHoist pass is ready for prime time yet and needs more testing, like we did for other passes that were first rigorously tested under a flag before being enabled by default. Going directly from zero to default with a major new optimization infrastructure (MemSSA) is a recipe for disaster.

Sadly the test case is fairly large and being a compile time regression reduction is difficult, I believe Alina is working on that.

sebpop added a subscriber: sebpop.Jul 21 2016, 1:07 PM

We already have a flag to disable the GVN-hoist pass:

static cl::opt<int>

MaxHoistedThreshold("gvn-max-hoisted", cl::Hidden, cl::init(-1),
                    cl::desc("Max number of instructions to hoist "
                             "(default unlimited = -1)"));

Please add "-mllvm -gvn-max-hoisted=0" to the failing tests.

To clarify, the tests in question are not upstreamed to the LLVM test suite, so there's nothing to disable here.
There is currently a work-around in Halide that is fairly ugly IMO (https://github.com/halide/Halide/blob/master/src/CodeGen_LLVM.cpp#L971).
Halide explicitly creates and invokes the PassManagerBuilder, so that's where having the disable option would be useful.

asbirlea updated this revision to Diff 64953.Jul 21 2016, 1:46 PM

Reverting previous update.
gvn-max-hoisted can be used as a command-line flag. Keep only the PassManagerBuilder flag while the pass gains maturity.

Note that the pass is still enabled by default.

chandlerc edited edge metadata.Jul 21 2016, 1:50 PM

We already have a flag to disable the GVN-hoist pass:

static cl::opt<int>

MaxHoistedThreshold("gvn-max-hoisted", cl::Hidden, cl::init(-1),
                    cl::desc("Max number of instructions to hoist "
                             "(default unlimited = -1)"));

Please add "-mllvm -gvn-max-hoisted=0" to the failing tests.

IMO, this is not the appropriate way to disable things not ready for prime time.

We should typically be disabling passes with a commandline flag *and* an option in the PassManagerBuilder because without that, library users of LLVM like Halide and others are left out in the cold.

That said, I certainly agree with Mehdi that we should design a better interface to the flags used by the PassManagerBuilder, but I don't think we should do that right now -- the goal here is to stop hurting users of LLVM that are completely broken by a new pass.

(And IMO, it should be disabled by default, and it should get enabled after reports from a diverse group of users report success. It certainly shouldn't be enabled by default weeks before a release branch.)

The problem is not a Halide test but a scalability issue in MemorySSA that's making Halide unusable, consuming gigabytes of memory and taking forever. Personally, I don't think the GVNHoist pass is ready for prime time yet and needs more testing, like we did for other passes that were first rigorously tested under a flag before being enabled by default. Going directly from zero to default with a major new optimization infrastructure (MemSSA) is a recipe for disaster.

I agree with all of that and I missed the fact that it was not the regular plain old GVN that we were talking here but a new (and apparently still "experimental") pass.
With this in mind, a cl::opt flag seem appropriate and I waive my previous concern.
Thanks for the extra information!

hans added a subscriber: hans.Jul 22 2016, 11:08 AM

Created PR28670.

hans added a comment.Jul 22 2016, 11:44 AM

It sounds like there is enough support for disabling gvn-hoist on trunk, merging that to 3.9, and re-enabling on trunk when things seem more stable.

Is anyone doing a patch for disabling it? How about https://reviews.llvm.org/differential/diff/65116/?

Hans, do you want to submit/commit the patch you suggested or should I update this one?
Either option is ok, just let me know.

hans added a comment.Jul 22 2016, 11:57 AM

Hans, do you want to submit/commit the patch you suggested or should I update this one?
Either option is ok, just let me know.

I'll be travelling for the rest of the day, so it's faster if you commit it.

asbirlea updated this revision to Diff 65142.Jul 22 2016, 1:47 PM
asbirlea edited edge metadata.

Updating with Hans's suggested diff.

mehdi_amini accepted this revision.Jul 22 2016, 2:53 PM
mehdi_amini added a reviewer: mehdi_amini.

LGTM.
Commit in the release_39 branch as well.

This revision is now accepted and ready to land.Jul 22 2016, 2:53 PM
This revision was automatically updated to reflect the committed changes.

This means to enable the pass in opt I need to do both -enable-gvn-hoist and -O1 which is not usual for opt. (For example I need just -dce to get DCE.) Is that the intent?

hans added a comment.Jul 29 2016, 9:19 AM

This means to enable the pass in opt I need to do both -enable-gvn-hoist and -O1 which is not usual for opt. (For example I need just -dce to get DCE.) Is that the intent?

I thought opt didn't use PassManagerBuilder, but added passes through some other magic. I don't actually know that this is how it works, but I assumed it was something like it, because test/Transforms/GVN/hoist.ll still just runs "opt -gvn-hoist" and it seems to work.

It does both as far as I know. Add a pass => it'll add that. Add an optimization level => it'll use the builder, and then the -enable flag has meaning.