Add flag to PassManagerBuilder to disable GVN Hoist Pass.

[asbirlea]

Summary

Adding a flag to diable GVN Hoisting.
Note: The GVN Hoist Pass causes some Halide tests to hang. Halide will disable the pass while investigating.

[davide]

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"));

[asbirlea]

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]

Add opt flag.

[mehdi_amini]

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?

[mehdi_amini]

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?

[bkramer]

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]

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.

[asbirlea]

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]

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]

In D22639#491781, @sebpop wrote:

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.)

[mehdi_amini]

In D22639#491753, @bkramer wrote:

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!

[asbirlea]

Created PR28670.

[hans]

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/?

[asbirlea]

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]

In D22639#493157, @asbirlea wrote:

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]

Updating with Hans's suggested diff.

[mehdi_amini]

LGTM.
Commit in the release_39 branch as well.

[probinson]

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]

In D22639#500689, @probinson wrote:

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.

[asbirlea]

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.

include/llvm/Transforms/IPO/PassManagerBuilder.h

public:
 
   /// Inliner - Specifies the inliner to use.  If this is non-null, it is
   /// added to the per-module passes.
   Pass *Inliner;
 
   /// The module summary index to use for function importing.
   const ModuleSummaryIndex *ModuleSummary;
 
+  bool DisableGVNHoist;
   bool DisableTailCalls;
   bool DisableUnitAtATime;
   bool DisableUnrollLoops;
   bool BBVectorize;
   bool SLPVectorize;
   bool LoopVectorize;
   bool RerollLoops;
   bool LoadCombine;

lib/Transforms/IPO/PassManagerBuilder.cpp

cl::desc("Control the amount of inlining in pre-instrumentation inliner "
              "(default = 75)"));
 
 PassManagerBuilder::PassManagerBuilder() {
     OptLevel = 2;
     SizeLevel = 0;
     LibraryInfo = nullptr;
     Inliner = nullptr;
     ModuleSummary = nullptr;
+    DisableGVNHoist = false;
     DisableUnitAtATime = false;
     DisableUnrollLoops = false;
     BBVectorize = RunBBVectorization;
     SLPVectorize = RunSLPVectorization;
     LoopVectorize = RunLoopVectorization;
     RerollLoops = RunLoopRerolling;
     LoadCombine = RunLoadCombine;
     DisableGVNLoadPRE = false;
void PassManagerBuilder::populateFunctionPassManager(
 
   if (OptLevel == 0) return;
 
   addInitialAliasAnalysisPasses(FPM);
 
   FPM.add(createCFGSimplificationPass());
   FPM.add(createSROAPass());
   FPM.add(createEarlyCSEPass());
+  if(!DisableGVNHoist)
     FPM.add(createGVNHoistPass());
   FPM.add(createLowerExpectIntrinsicPass());
 }
 
 // Do PGO instrumentation generation or use pass as the option specified.
 void PassManagerBuilder::addPGOInstrPasses(legacy::PassManagerBase &MPM) {
   if (PGOInstrGen.empty() && PGOInstrUse.empty())
     return;
   // Perform the preinline and cleanup passes for O1 and above.