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

[Quentin Colombet]

Hi Duncan,

On Mar 19, 2013, at 12:52 PM, Duncan Sands <baldrick at free.fr> wrote:

> 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.
Yes, you’re right, it shouldn’t be needed anymore.

> 
>>  #include "llvm/IR/Instructions.h"
>>  #include "llvm/IR/Intrinsics.h"
>> +#include "llvm/IR/IntrinsicInst.h"
> 
> Is this include needed?
No, it shouldn’t.

> 
>>  #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?
As it is if you enable it by default, the heuristic (merge variable by size) produces performance regression already noticeable by during make check.
I didn’t want to impact any existing target using the global merge pass, thus I’ve kept the original behavior by default.
The idea is to improve the current heuristic as a second step, then enable it by default.


> 
>> +
>>  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.
Ok

>> +    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?
Yes, it is.
Where would be the right place for such helper?
> 
>> +
>> +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.
Although it is not currently happening, it is possible in the future, especially with the new design of the pass manager that allows to reuse passes for different objects (Function, Module, etc.)
Anyway, to follow the intended design, I'll put this piece of code in doFinalization.

> 
>> +  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.
Ok

> 
>> +    if (isMustKeepGlobalVariable(I))
>> +      continue;
>> +
>>      if (TD->getTypeAllocSize(Ty) < MaxOffset) {
>>        if (TargetLoweringObjectFile::getKindForGlobal(I, TLI->getTargetMachine())
>>            .isBSSLocal())
I’ve made a patch with the straight forward changes you’ve suggested: r177445.
I’m waiting for direction on where to place the helper function to collect the used variables.

Thanks for your help.

-Quentin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130319/c08b64d2/attachment.html>