[llvm] r177331 - Extend global merge pass to optionally consider global constant variables.

[Duncan Sands]

Hi Quentin,

On 18/03/13 23:30, Quentin Colombet wrote:
> Author: qcolombet
> Date: Mon Mar 18 17:30:07 2013
> New Revision: 177331
>
> URL: http://llvm.org/viewvc/llvm-project?rev=177331&view=rev
> Log:
> Extend global merge pass to optionally consider global constant variables.
> Also add some checks to not merge globals used within landing pad instructions or marked as "used".
>
> Modified:
>      llvm/trunk/lib/Transforms/Scalar/GlobalMerge.cpp
>      llvm/trunk/test/CodeGen/ARM/global-merge.ll
>
> Modified: llvm/trunk/lib/Transforms/Scalar/GlobalMerge.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GlobalMerge.cpp?rev=177331&r1=177330&r2=177331&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/GlobalMerge.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/GlobalMerge.cpp Mon Mar 18 17:30:07 2013
> @@ -53,6 +53,7 @@
>
>   #define DEBUG_TYPE "global-merge"
>   #include "llvm/Transforms/Scalar.h"
> +#include "llvm/ADT/SmallPtrSet.h"
>   #include "llvm/ADT/Statistic.h"
>   #include "llvm/IR/Attributes.h"
>   #include "llvm/IR/Constants.h"
> @@ -60,14 +61,22 @@
>   #include "llvm/IR/DerivedTypes.h"
>   #include "llvm/IR/Function.h"
>   #include "llvm/IR/GlobalVariable.h"
> +#include "llvm/IR/InlineAsm.h"

I guess this include is not needed.

>   #include "llvm/IR/Instructions.h"
>   #include "llvm/IR/Intrinsics.h"
> +#include "llvm/IR/IntrinsicInst.h"

Is this include needed?

>   #include "llvm/IR/Module.h"
>   #include "llvm/Pass.h"
> +#include "llvm/Support/CommandLine.h"
>   #include "llvm/Target/TargetLowering.h"
>   #include "llvm/Target/TargetLoweringObjectFile.h"
>   using namespace llvm;
>
> +static cl::opt<bool>
> +EnableGlobalMergeOnConst("global-merge-on-const", cl::Hidden,
> +                  	cl::desc("Enable global merge pass on constants"),
> +                  	cl::init(false));

Why is this optional, why not always do it?

> +
>   STATISTIC(NumMerged      , "Number of globals merged");
>   namespace {
>     class GlobalMerge : public FunctionPass {
> @@ -78,6 +87,23 @@ namespace {
>       bool doMerge(SmallVectorImpl<GlobalVariable*> &Globals,
>                    Module &M, bool isConst, unsigned AddrSpace) const;
>
> +    /// \brief Check if the given variable has been identified as must keep
> +    /// \pre setMustKeepGlobalVariables must have been called on the Module that
> +    ///      contains GV
> +    bool isMustKeepGlobalVariable(const GlobalVariable *GV) const {
> +      return MustKeepGlobalVariables.count(GV);
> +    }
> +
> +    /// Collect every variables marked as "used" or used in a landing pad
> +    /// instruction for this Module.
> +    void setMustKeepGlobalVariables(Module &M);
> +
> +    /// Collect every variables marked as "used"
> +    void collectUsedGlobalVariables(Module &M);
> +
> +    /// Keep track of the GlobalVariable that are marked as "used"

This comment is misleading since it isn't just for "used" variables.  How
about:
   /// Keep track of global variables that must not be merged away.
> +    SmallPtrSet<const GlobalVariable *, 16> MustKeepGlobalVariables;
> +
>     public:
>       static char ID;             // Pass identification, replacement for typeid.
>       explicit GlobalMerge(const TargetLowering *tli = 0)
> @@ -169,6 +195,46 @@ bool GlobalMerge::doMerge(SmallVectorImp
>     return true;
>   }
>
> +void GlobalMerge::collectUsedGlobalVariables(Module &M) {
> +  // Extract global variables from llvm.used array
> +  const GlobalVariable *GV = M.getGlobalVariable("llvm.used");
> +  if (!GV || !GV->hasInitializer()) return;
> +
> +  // Should be an array of 'i8*'.
> +  const ConstantArray *InitList = dyn_cast<ConstantArray>(GV->getInitializer());
> +  if (InitList == 0) return;
> +
> +  for (unsigned i = 0, e = InitList->getNumOperands(); i != e; ++i)
> +    if (const GlobalVariable *G =
> +        dyn_cast<GlobalVariable>(InitList->getOperand(i)->stripPointerCasts()))
> +      MustKeepGlobalVariables.insert(G);
> +}

The initial part of the above logic is probably common to every place that
examines llvm.used.  Maybe you could factor it out into a shared helper
function?

> +
> +void GlobalMerge::setMustKeepGlobalVariables(Module &M) {
> +  // If we already processed a Module, UsedGlobalVariables may have been
> +  // populated. Reset the information for this module.

Is the above really possible?  I thought a new instance of the pass was created
every time it is run on a module.

> +  MustKeepGlobalVariables.clear();
> +  collectUsedGlobalVariables(M);
> +
> +  for (Module::iterator IFn = M.begin(), IEndFn = M.end(); IFn != IEndFn;
> +       ++IFn) {
> +    for (Function::iterator IBB = IFn->begin(), IEndBB = IFn->end();
> +         IBB != IEndBB; ++IBB) {
> +      // Follow the inwoke link to find the landing pad instruction
> +      const InvokeInst *II = dyn_cast<InvokeInst>(IBB->getTerminator());
> +      if (!II) continue;
> +
> +      const LandingPadInst *LPInst = II->getUnwindDest()->getLandingPadInst();
> +      // Look for globals in the clauses of the landing pad instruction
> +      for (unsigned Idx = 0, NumClauses = LPInst->getNumClauses();
> +           Idx != NumClauses; ++Idx)
> +        if (const GlobalVariable *GV =
> +            dyn_cast<GlobalVariable>(LPInst->getClause(Idx)
> +                                     ->stripPointerCasts()))
> +          MustKeepGlobalVariables.insert(GV);
> +    }
> +  }
> +}
>
>   bool GlobalMerge::doInitialization(Module &M) {
>     DenseMap<unsigned, SmallVector<GlobalVariable*, 16> > Globals, ConstGlobals,
> @@ -176,6 +242,7 @@ bool GlobalMerge::doInitialization(Modul
>     const DataLayout *TD = TLI->getDataLayout();
>     unsigned MaxOffset = TLI->getMaximalGlobalOffset();
>     bool Changed = false;
> +  setMustKeepGlobalVariables(M);
>
>     // Grab all non-const globals.
>     for (Module::global_iterator I = M.global_begin(),
> @@ -200,6 +267,12 @@ bool GlobalMerge::doInitialization(Modul
>           I->getName().startswith(".llvm."))
>         continue;
>
> +    // Ignore all "required" globals:
> +    // - the ones used for EH
> +    // - the ones marked with "used" attribute

I think it is a mistake to explain here the logic used to define
MustKeepGlobalVariables.  I think it would be better to just say:
   // Ignore all "required" globals.

> +    if (isMustKeepGlobalVariable(I))
> +      continue;
> +
>       if (TD->getTypeAllocSize(Ty) < MaxOffset) {
>         if (TargetLoweringObjectFile::getKindForGlobal(I, TLI->getTargetMachine())
>             .isBSSLocal())

Ciao, Duncan.