[llvm-commits] cxa_guard elimination

[Chris Lattner]

On Feb 9, 2012, at 10:21 AM, Nick Lewycky wrote:
>> Of course GlobalOpt is interprocedural, since it already has to not be
>> fouled up by constructor calls.
>> 
>> Updated patch attached, including more bugfixes than ever before! Please
>> review!
> 
> Updated patch again, now without memory corruption. Also slightly more aggressive by ignoring AllocaTmps when scanning through ReadMemory and MutatedMemory.
> 
> It turns out that llvm-test is not a good test suite for this particular change.


Ok.  Some thoughts:

     virtual void getAnalysisUsage(AnalysisUsage &AU) const {
+      AU.addRequired<DominatorTree>();

Is this *really really* required?  This will be the first module pass that requires a function pass in mainline IIRC.  Does the passmanager only compute DominatorTree for functions lazily when asking for it?  Are you sure that GlobalOpt isn't changing CFGs in a way that would invalidate DomTree?

Would it be enough to just chase up single predecessors or something like that?


@@ -273,6 +283,25 @@
         assert(MSI->getArgOperand(0) == V && "Memset only takes one pointer!");
         if (MSI->isVolatile()) return true;
         GS.StoredType = GlobalStatus::isStored;
+      } else if (const CallInst *CI = dyn_cast<CallInst>(I)) {
+        Function *Callee = CI->getCalledFunction();
+        if (!Callee)
+          return true;

Please add a comment here along the lines of: "Check to see if this global is being used as an initializer guard by seeing if it is passed into __cxa_guard_acquire/release".

I don't have a strong opinion on this, but does this need to use TargetLibraryInfo to disable the transformation with -fno-builtin?

It's not your bug, but that loop really really should use 'continue' to make the control flow more obvious.

+static Instruction *UseToInst(Value *V) {

Perhaps this is prevented by how it is called, but you can have an infinite constant loop through a global variable and its initializer.


The EvaluateBlock/EvaluateFunction interfaces really need to be split out into methods on a helper class that maintain the state, instead of passing the state through a dozen arguments into each recursive call.  Can you split this out into its own patch?


There are various blocks of logic in OptimizeGuardedInitialization that really need some block comments explaining what is going on :)


      InstResult = ConstantExpr::get(BO->getOpcode(),
-                                     getVal(Values, BO->getOperand(0)),
-                                     getVal(Values, BO->getOperand(1)));
+      Constant *Op0 = getVal(Values, BO->getOperand(0));
+      Constant *Op1 = getVal(Values, BO->getOperand(1));
+      if (!Op0 || !Op1) return false;

It's hard for me to understand what is going on here.  Why did getVal suddenly start being able to fail?  Can you split this out into its own patch?

Overall, I'm really glad you've tackled this.  It would help a lot if you could do some of the other patches to make it more clear and straight-forward.  It's hard to understand the actual transformation as is :)

-Chris