[llvm-commits] [Patch] Make GlobalOpt conservative with TLS vars (PR14309, 3.2-blocker)

[Nick Lewycky]

Hans Wennborg wrote:
> Hi all,
>
> The attached patch takes a stab at fixing PR14309 which was marked as
> a 3.2-blocker.
>
> For global variables which get the same value stored into them
> everywhere, GlobalOpt will replace them with a constant. The problem
> is that a thread-local GlobalVariable looks like one value, but is
> different between threads.

Thanks for looking at this!

+/// isThreadDependent - Return true if the value can vary between threads.
+bool Constant::isThreadDependent() const {
+  if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(this))
+    return GV->isThreadLocal();
+
+  for (unsigned I = 0, E = getNumOperands(); I != E; ++I) {
+    if (const Constant *C = dyn_cast<Constant>(getOperand(I))) {

Use cast<> instead of dyn_cast<> here. Constant operands are always 
constants.

+      if (C->isThreadDependent())

Two things. Firstly, I'm worried about stack depth, so I'd rather this 
function maintained its own worklist. Secondly, I'm worried about 
callers using this as part of their own constant visiting and 
accidentally forming an O(n^2) loop, because this function doesn't 
expose a cache or anything. You needn't do anything about point #2 
though, there are many places in llvm to fall into that particular trap.

+        return true;
+    }
+  }
+
+  return false;
+}

@@ -234,6 +235,14 @@ static bool AnalyzeGlobal(const Value *V, 
GlobalStatus &GS,
            if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(
 
SI->getOperand(1))) {
              Value *StoredVal = SI->getOperand(0);
+
+            if (Constant *C = dyn_cast<Constant>(StoredVal)) {
+              if (C->isThreadDependent()) {
+                // The stored value changes between threads; don't 
track it.
+                return true;
+              }
+            }
+
              if (StoredVal == GV->getInitializer()) {
                if (GS.StoredType < GlobalStatus::isInitializerStored)
                  GS.StoredType = GlobalStatus::isInitializerStored;

This is really scary. I didn't understand why you needed to to this 
until I understood the test. (I thought the bug was that we weren't 
copying the TLS bit to the new global at line 1819. Nope!)

> My patch introduces Constant::isThreadDependent() which returns true
> for thread-local variables and constants which depend on them (e.g. a
> GEP into a thread-local array), and teaches GlobalOpt not to track
> such values.

> Please take a look!
>
> Also, if anyone thinks there are more passes which assume that
> thread-local GlobalVariables have the same value everywhere, please
> let me know.

The memdep users for a start; memdep itself, gvn, dse. I think what's 
been saving us is that nobody marks their "create a thread" function as 
readonly.

I think this is just another reason we need to kill ConstantExpr, and 
forbid expressions involving TLS to be constant.

Nick