[GVN] introduce GVNOptions to control GVN pass behavior

[fedor.sergeev]

Summary

There are a few global (cl::opt) controls that enable optional
behavior in GVN. Introduce GVNOptions that provide corresponding
per-pass instance controls.

That will allow to use GVN multiple times in pipeline each time
with different settings.

[asbirlea]

Thanks, this looks useful!

llvm/include/llvm/Transforms/Scalar/GVN.h

90

Update comment?

llvm/lib/Transforms/Scalar/GVN.cpp

636

Conditionally getResult<MemDepAnalysis> in a follow up change?

[skatkov]

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

[fedor.sergeev]

updated comments

[fedor.sergeev]

llvm/lib/Transforms/Scalar/GVN.cpp

636

Yep, will do as a separate one-liner after this patch.

[fedor.sergeev]

In D72732#1821154, @skatkov wrote:

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

That should go as a followup.
I did not plan to do that right now, but well, you got a point.
Will post a review for that.

[skatkov]

In D72732#1821201, @fedor.sergeev wrote:

In D72732#1821154, @skatkov wrote:

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

That should go as a followup.
I did not plan to do that right now, but well, you got a point.
Will post a review for that.

I'm fine with follow-up.

[gchatelet]

Removing myself from the reviewers (I'm not an owner of this code)

[fhahn]

llvm/lib/Transforms/Scalar/GVN.cpp

2725

This could use Impl. isMemDepEnabled as well, right? Then we could get rid of the NoMemDepAnalysis variable.

[fedor.sergeev]

fixing NoMemDepAnalysis use in legacy pass

[fhahn]

LGTM, thanks. Please wait a day or so with committing, in case there are additional comments.

[fedor.sergeev]

llvm/lib/Transforms/Scalar/GVN.cpp

2696–2698

Also removed default value as it is not used anyway and just gives a wrong feeling that global control affects something here.
In fact it was not (and I just kept pre-existing behavior).

2725

thanks for catching this!

[fedor.sergeev]

reformatting after the last update

[fedor.sergeev]

llvm/lib/Transforms/Scalar/GVN.cpp

636

since this patch became rather involved on legacy part anyway,
I decided that doing this change here (and not in a followup) is fine.

[fhahn]

llvm/lib/Transforms/Scalar/GVN.cpp

636

Sounds good to me.

[fedor.sergeev]

reintroduced default parameter for Legacy pass constructor
(it is necessary for PassSupport.h proper operations and not otherwise used)

llvm/include/llvm/Transforms/Scalar/GVN.h

 namespace gvn LLVM_LIBRARY_VISIBILITY {
 
 struct AvailableValue;
 struct AvailableValueInBlock;
 class GVNLegacyPass;
 
 } // end namespace gvn
 
+/// A set of parameters to control various transforms performed by GVN pass.
+//  Each of the optional boolean parameters can be set to:
+///      true - enabling the transformation.
+///      false - disabling the transformation.
+///      None - relying on a global default.
+/// Intended use is to create a default object, modify parameters with
+/// additional setters and then pass it to GVN.
+struct GVNOptions {
+  Optional<bool> AllowPRE = None;
+  Optional<bool> AllowLoadPRE = None;
+  Optional<bool> AllowMemDep = None;
+
+  GVNOptions() = default;
+
+  /// Enables or disables PRE in GVN.
+  GVNOptions &setPRE(bool PRE) {
+    AllowPRE = PRE;
+    return *this;
+  }
+
+  /// Enables or disables PRE of loads in GVN.
+  GVNOptions &setLoadPRE(bool LoadPRE) {
+    AllowLoadPRE = LoadPRE;
+    return *this;
+  }
+
+  /// Enables or disables PRE of loads in GVN.
+  GVNOptions &setMemDep(bool MemDep) {
+    AllowMemDep = MemDep;
+    return *this;
+  }
+};
+
 /// The core GVN pass object.
 ///
 /// FIXME: We should have a good summary of the GVN algorithm implemented by
 /// this particular pass here.
 class GVN : public PassInfoMixin<GVN> {
+  GVNOptions Options;
+
 public:
   struct Expression;
 
+  GVN(GVNOptions Options = {}) : Options(Options) {}
+
   /// Run the pass over the function.
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
 
   /// This removes the specified instruction from
   /// our various maps and marks it for deletion.
   void markInstructionForDeletion(Instruction *I) {
     VN.erase(I);
     InstrsToErase.push_back(I);
   }
 
   DominatorTree &getDominatorTree() const { return *DT; }
   AliasAnalysis *getAliasAnalysis() const { return VN.getAliasAnalysis(); }
   MemoryDependenceResults &getMemDep() const { return *MD; }
 
+  bool isPREEnabled() const;
+  bool isLoadPREEnabled() const;
+  bool isMemDepEnabled() const;
+
   /// This class holds the mapping between values and value numbers.  It is used
   /// as an efficient mechanism to determine the expression-wise equivalence of
   /// two values.
   class ValueTable {
     DenseMap<Value *, uint32_t> valueNumbering;
     DenseMap<Expression, uint32_t> expressionNumbering;
 
     // Expressions is the vector of Expression. ExprIdx is the mapping from

llvm/lib/Transforms/Scalar/GVN.cpp

 STATISTIC(NumGVNInstr,  "Number of instructions deleted");
 STATISTIC(NumGVNLoad,   "Number of loads deleted");
 STATISTIC(NumGVNPRE,    "Number of instructions PRE'd");
 STATISTIC(NumGVNBlocks, "Number of blocks merged");
 STATISTIC(NumGVNSimpl,  "Number of instructions simplified");
 STATISTIC(NumGVNEqProp, "Number of equalities propagated");
 STATISTIC(NumPRELoad,   "Number of loads PRE'd");
 
-static cl::opt<bool> EnablePRE("enable-pre",
-                               cl::init(true), cl::Hidden);
-static cl::opt<bool> EnableLoadPRE("enable-load-pre", cl::init(true));
-static cl::opt<bool> EnableMemDep("enable-gvn-memdep", cl::init(true));
+static cl::opt<bool> GVNEnablePRE("enable-pre", cl::init(true), cl::Hidden);
+static cl::opt<bool> GVNEnableLoadPRE("enable-load-pre", cl::init(true));
+static cl::opt<bool> GVNEnableMemDep("enable-gvn-memdep", cl::init(true));
 
 // Maximum allowed recursion depth.
 static cl::opt<uint32_t>
 MaxRecurseDepth("gvn-max-recurse-depth", cl::Hidden, cl::init(1000), cl::ZeroOrMore,
                 cl::desc("Max recurse depth in GVN (default = 1000)"));
 
 static cl::opt<uint32_t> MaxNumDeps(
     "gvn-max-num-deps", cl::Hidden, cl::init(100), cl::ZeroOrMore,
for (DenseMap<Value*, uint32_t>::const_iterator
     assert(I->first != V && "Inst still occurs in value numbering map!");
   }
 }
 
 //===----------------------------------------------------------------------===//
 //                                GVN Pass
 //===----------------------------------------------------------------------===//
 
+bool GVN::isPREEnabled() const {
+  return Options.AllowPRE.getValueOr(GVNEnablePRE);
+}
+
+bool GVN::isLoadPREEnabled() const {
+  return Options.AllowLoadPRE.getValueOr(GVNEnableLoadPRE);
+}
+bool GVN::isMemDepEnabled() const {
+  return Options.AllowMemDep.getValueOr(GVNEnableMemDep);
+}
+
 PreservedAnalyses GVN::run(Function &F, FunctionAnalysisManager &AM) {
   // FIXME: The order of evaluation of these 'getResult' calls is very
   // significant! Re-ordering these variables will cause GVN when run alone to
   // be less effective! We should fix memdep and basic-aa to not exhibit this
   // behavior, but until then don't change the order here.
   auto &AC = AM.getResult<AssumptionAnalysis>(F);
   auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
   auto &TLI = AM.getResult<TargetLibraryAnalysis>(F);
   auto &AA = AM.getResult<AAManager>(F);
   auto &MemDep = AM.getResult<MemoryDependenceAnalysis>(F);
   auto *LI = AM.getCachedResult<LoopAnalysis>(F);
   auto &ORE = AM.getResult<OptimizationRemarkEmitterAnalysis>(F);
-  bool Changed = runImpl(F, AC, DT, TLI, AA, &MemDep, LI, &ORE);
+  bool Changed = runImpl(F, AC, DT, TLI, AA,
+                         isMemDepEnabled() ? &MemDep : nullptr, LI, &ORE);
   if (!Changed)
     return PreservedAnalyses::all();
   PreservedAnalyses PA;
   PA.preserve<DominatorTreeAnalysis>();
   PA.preserve<GlobalsAA>();
   PA.preserve<TargetLibraryAnalysis>();
   if (LI)
     PA.preserve<LoopAnalysis>();
if (V->getType()->isPtrOrPtrVectorTy())
       MD->invalidateCachedPointerInfo(V);
     markInstructionForDeletion(LI);
     ++NumGVNLoad;
     reportLoadElim(LI, V, ORE);
     return true;
   }
 
   // Step 4: Eliminate partial redundancy.
-  if (!EnablePRE || !EnableLoadPRE)
+  if (!isPREEnabled() || !isLoadPREEnabled())
     return false;
 
   return PerformLoadPRE(LI, ValuesPerBlock, UnavailableBlocks);
 }
 
 static bool impliesEquivalanceIfTrue(CmpInst* Cmp) {
   if (Cmp->getPredicate() == CmpInst::Predicate::ICMP_EQ)
     return true;
bool GVN::runImpl(Function &F, AssumptionCache &RunAC, DominatorTree &RunDT,
   unsigned Iteration = 0;
   while (ShouldContinue) {
     LLVM_DEBUG(dbgs() << "GVN iteration: " << Iteration << "\n");
     ShouldContinue = iterateOnFunction(F);
     Changed |= ShouldContinue;
     ++Iteration;
   }
 
-  if (EnablePRE) {
+  if (isPREEnabled()) {
     // Fabricate val-num for dead-code in order to suppress assertion in
     // performPRE().
     assignValNumForDeadCode();
     bool PREChanged = true;
     while (PREChanged) {
       PREChanged = performPRE(F);
       Changed |= PREChanged;
     }
for (BasicBlock *BB : DeadBlocks) {
     }
   }
 }
 
 class llvm::gvn::GVNLegacyPass : public FunctionPass {
 public:
   static char ID; // Pass identification, replacement for typeid
 
-  explicit GVNLegacyPass(bool NoMemDepAnalysis = !EnableMemDep)
-      : FunctionPass(ID), NoMemDepAnalysis(NoMemDepAnalysis) {
+  explicit GVNLegacyPass(bool NoMemDepAnalysis = !GVNEnableMemDep)
+      : FunctionPass(ID), NoMemDepAnalysis(NoMemDepAnalysis),
+        Impl(GVNOptions().setMemDep(!NoMemDepAnalysis)) {
     initializeGVNLegacyPassPass(*PassRegistry::getPassRegistry());
   }
 
   bool runOnFunction(Function &F) override {
     if (skipFunction(F))
       return false;
 
     auto *LIWP = getAnalysisIfAvailable<LoopInfoWrapperPass>();
return Impl.runImpl(
         &getAnalysis<OptimizationRemarkEmitterWrapperPass>().getORE());
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<AssumptionCacheTracker>();
     AU.addRequired<DominatorTreeWrapperPass>();
     AU.addRequired<TargetLibraryInfoWrapperPass>();
     AU.addRequired<LoopInfoWrapperPass>();
     if (!NoMemDepAnalysis)
       AU.addRequired<MemoryDependenceWrapperPass>();
     AU.addRequired<AAResultsWrapperPass>();
 
     AU.addPreserved<DominatorTreeWrapperPass>();
     AU.addPreserved<GlobalsAAWrapperPass>();
     AU.addPreserved<TargetLibraryInfoWrapperPass>();
     AU.addPreserved<LoopInfoWrapperPass>();
     AU.addPreservedID(LoopSimplifyID);