[PATCH] Global merge on constants

[Bill Wendling]

On Mar 18, 2013, at 1:28 PM, Quentin Colombet <qcolombet at apple.com> wrote:

> Thanks for the feedbacks Duncan.
> 
> Here is the updated version of the patch as well as the updated compile time numbers.
> The updated patch just checks for the “used” attribute and the uses in landing pad instructions. More particularly, it looks for all landing pad instructions in the module and marks internally the involved global variables as “must keep”.
> 
> For the compile time numbers, on average performing the new checks plus merging the constants (SPEC-compile_time_with_const.txt) does not have any noticeable impact on the compile time now.
>  
Looking good! One comment:

+void GlobalMerge::setMustKeepGlobalVariables(Module &M) {
+  // If we already processed a Module, UsedGlobalVariables may have been
+  // populated. Reset the information for this 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) {
+      const LandingPadInst *LPInst = IBB->getLandingPadInst();
+      if (!LPInst)
+        continue;

As a micro-optimization, you might want to rewrite this as:

	const InvokeInst *II = dyn_cast<InvokeInst>(IBB->getTerminator());
	if (!II) continue;

	const LandingPadInst *LPInst = II->getUnwindDest()->getLandingPadInst();

This way it's not looping through the beginning of a basic blocks unless it has to.

+      // Look for globals in the operands of the landing pad instruction
+      for (unsigned OpIdx = 0, EndOpIdx = LPInst->getNumOperands();
+           OpIdx != EndOpIdx; ++OpIdx) {

Instead of looping through all operands, you might want to concentrate on just the clauses:

	for (unsigned Idx = 0, NumClauses = LPInst->getNumClauses(); Idx != NumClauses; ++Idx)
	  if (const GlobalVariable *GV =
	 	dyn_cast<GlobalVariable>(LPInst->getClause(Idx)->stripPointerCasts()))
	    MustKeepGlobalVariables.insert(GV);

The other uses of globals inside of the LandingPadInst shouldn't be candidates for merging already (or if they are, it's not a problem).

+        const GlobalVariable *GV =
+          dyn_cast<GlobalVariable>(LPInst->getOperand(OpIdx)->
+                                   stripPointerCasts());
+        if (GV)
+          MustKeepGlobalVariables.insert(GV);
+      }
+    }
+  }
+}

-bw