[llvm-branch-commits] [llvm] [GVN] More restructuring of `GVN.h` to reduce its size (NFC) (PR #211541)

[Momchil Velikov via llvm-branch-commits]

================
@@ -123,197 +123,25 @@ struct GVNOptions {
 ///
 /// FIXME: We should have a good summary of the GVN algorithm implemented by
 /// this particular pass here.
+class GVNPassImpl;
 class GVNPass : public OptionalPassInfoMixin<GVNPass> {
-public:
-  struct AvailableValue;
-  struct AvailableValueInBlock;
-  struct ReachingMemVal;
-  struct DependencyBlockInfo;
-
-  friend class GVNValueTable;
-  friend class GVNLegacyPass;
-
-private:
-  GVNOptions Options;
-  MemoryDependenceResults *MD = nullptr;
-  DominatorTree *DT = nullptr;
-  const TargetLibraryInfo *TLI = nullptr;
-  AssumptionCache *AC = nullptr;
-  SetVector<BasicBlock *> DeadBlocks;
-  OptimizationRemarkEmitter *ORE = nullptr;
-  ImplicitControlFlowTracking *ICF = nullptr;
-  LoopInfo *LI = nullptr;
-  AAResults *AA = nullptr;
-  MemorySSAUpdater *MSSAU = nullptr;
-  GVNValueTable VN;
-  GVNLeaderMap LeaderTable;
-
-  // Map the block to reversed postorder traversal number. It is used to
-  // find back edge easily.
-  DenseMap<AssertingVH<BasicBlock>, uint32_t> BlockRPONumber;
-
-  // This is set 'true' initially and also when new blocks have been added to
-  // the function being analyzed. This boolean is used to control the updating
-  // of BlockRPONumber prior to accessing the contents of BlockRPONumber.
-  bool InvalidBlockRPONumbers = true;
-
-  // List of critical edges to be split between iterations.
-  SmallVector<std::pair<Instruction *, unsigned>, 4> ToSplit;
+  std::unique_ptr<GVNPassImpl> Impl;
----------------
momchil-velikov wrote:

I don't particularly mind the approach with a local object.

But ...

> Simpler code, no need to explicitly define/delete copy/move operations and destructors.

I don't think these defaulted/deleted member functions add much or any complexity.

>  More similarity with other passes is also a win.

It's a win, all else equal.

> Similar to other passes, the pass should just store GVNOptions. See e.g. LICMPass, SimplifyCFGPass, SROAPass, NewGVNPass, ...

It cannot just store the options, they are consulted during running.

I understand the sentiment of keep doing the same thing, but I'd rather hear a clearly articulated compelling technical argument why a particular design decision is better that this one, for this pass and for others. 

Imagine if the alternative approach was initially just arbitrary, and then got perpetuated across e.g. LICMPass, SimplifyCFGPass, SROAPass, NewGVNPass, ... and now maybe GVN ?





https://github.com/llvm/llvm-project/pull/211541