[PATCH] D14148: [GlobalOpt] Demote globals to locals more aggressively

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Nov-13, at 04:38, James Molloy <james.molloy at arm.com> wrote:
> 
> jmolloy updated this revision to Diff 40137.
> jmolloy added a comment.
> 
> Hi Mehdi, Duncan, Manman,
> 
> I've updated the patch following all your comments. Implementing Mehdi's suggestion made the code look significantly neater and will also catch more cases.
> 
> I've also undone the reordering of optimizations in ProcessInternalGlobal, as I don't think this is now needed now that "main" isn't implicitly norecurse. It was only done to pacify tests in the first place, and they all pass now without modification.
> 
> Cheers,
> 
> James
> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D14148
> 
> Files:
>  lib/Transforms/IPO/GlobalOpt.cpp
>  test/Transforms/GlobalOpt/global-demotion.ll
>  test/Transforms/GlobalOpt/metadata.ll
>  test/Transforms/MergeFunc/crash2.ll
> 
> <D14148.40137.patch>

This patch looks way cleaner.

Just a few more nitpicks; otherwise LGTM once Manman and Mehdi are happy.

> Index: lib/Transforms/IPO/GlobalOpt.cpp
> ===================================================================
> --- lib/Transforms/IPO/GlobalOpt.cpp
> +++ lib/Transforms/IPO/GlobalOpt.cpp
> @@ -28,6 +28,7 @@
>  #include "llvm/IR/Constants.h"
>  #include "llvm/IR/DataLayout.h"
>  #include "llvm/IR/DerivedTypes.h"
> +#include "llvm/IR/Dominators.h"
>  #include "llvm/IR/GetElementPtrTypeIterator.h"
>  #include "llvm/IR/Instructions.h"
>  #include "llvm/IR/IntrinsicInst.h"
> @@ -69,6 +70,7 @@
>    struct GlobalOpt : public ModulePass {
>      void getAnalysisUsage(AnalysisUsage &AU) const override {
>        AU.addRequired<TargetLibraryInfoWrapperPass>();
> +      AU.addRequired<DominatorTreeWrapperPass>();
>      }
>      static char ID; // Pass identification, replacement for typeid
>      GlobalOpt() : ModulePass(ID) {
> @@ -85,7 +87,9 @@
>      bool ProcessInternalGlobal(GlobalVariable *GV,Module::global_iterator &GVI,
>                                 const GlobalStatus &GS);
>      bool OptimizeEmptyGlobalCXXDtors(Function *CXAAtExitFn);
> -
> +    

There are spaces on the new version of this empty line.  clang-format?

> +    bool isPointerValueDeadOnEntryToFunction(const Function *F, GlobalValue *GV);
> +    

There are spaces on this empty line too.

>      TargetLibraryInfo *TLI;
>      SmallSet<const Comdat *, 8> NotDiscardableComdats;
>    };
> @@ -95,6 +99,7 @@
>  INITIALIZE_PASS_BEGIN(GlobalOpt, "globalopt",
>                  "Global Variable Optimizer", false, false)
>  INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
> +INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
>  INITIALIZE_PASS_END(GlobalOpt, "globalopt",
>                  "Global Variable Optimizer", false, false)
>  
> @@ -1718,26 +1723,90 @@
>    return ProcessInternalGlobal(GV, GVI, GS);
>  }
>  
> +bool GlobalOpt::isPointerValueDeadOnEntryToFunction(const Function *F, GlobalValue *GV) {
> +  // Find all uses of GV. We expect them all to be in F, and if we can't
> +  // identify any of the uses we bail out.
> +  //
> +  // On each of these uses, identify if the memory that GV points to is
> +  // used/required/live at the start of the function. If it is not, for example
> +  // if the first thing the function does is store to the GV, the GV can
> +  // possibly be demoted.
> +  //
> +  // We don't do an exhaustive search for memory operations - simply look
> +  // through bitcasts as they're quite common and benign.
> +  const DataLayout &DL = GV->getParent()->getDataLayout();
> +  SmallVector<LoadInst *, 4> Loads;
> +  SmallVector<StoreInst *, 4> Stores;
> +  for (auto *U : GV->users()) {
> +    if (Operator::getOpcode(U) == Instruction::BitCast) {
> +      for (auto *UU : U->users()) {
> +        if (auto *LI = dyn_cast<LoadInst>(UU))
> +          Loads.push_back(LI);
> +        else if (auto *SI = dyn_cast<StoreInst>(UU))
> +          Stores.push_back(SI);
> +        else
> +          return false;
> +      }
> +      continue;
> +    }
> +
> +    Instruction *I = dyn_cast<Instruction>(U);
> +    if (!I)
> +      return false;
> +    assert(I->getParent()->getParent() == F);
> +
> +    if (auto *LI = dyn_cast<LoadInst>(I))
> +      Loads.push_back(LI);
> +    else if (auto *SI = dyn_cast<StoreInst>(I))
> +      Stores.push_back(SI);
> +    else
> +      return false;
> +  }
> +
> +  // We have identified all uses of GV into loads and stores. Now check if all
> +  // of them are known not to depend on the value of the global at the function
> +  // entry point. We do this by ensuring that every load is dominated by at
> +  // least one store.
> +  auto &DT = getAnalysis<DominatorTreeWrapperPass>(*const_cast<Function *>(F))
> +                 .getDomTree();
> +
> +  for (auto *L : Loads) {
> +    auto *LTy = L->getType();
> +    if (!std::any_of(Stores.begin(), Stores.end(), [&](StoreInst *S) {

This looks quadratic.  Should we give up if there are too many loads and
stores?

> +          auto *STy = S->getValueOperand()->getType();
> +          // The load is only dominated by the store if DomTree says so
> +          // and the number of bits loaded in L is less than or equal to
> +          // the number of bits stored in S.
> +          return DT.dominates(S, L) &&
> +                 DL.getTypeStoreSize(LTy) <= DL.getTypeStoreSize(STy);
> +        }))
> +      return false;
> +  }
> +  // All loads have known dependences inside F, so the global can be localized.
> +  return true;
> +}