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

[Nick Lewycky]

Hans Wennborg wrote:
> Thanks for the review, Nick! Attaching new patch addressing your comments.

+    const Constant *C = WorkList.back();
+    WorkList.pop_back();

const Constant *C = WorkList.pop_back_val();

+      if (!Visited.count(D)) {
+        WorkList.push_back(D);
+        Visited.insert(C);
+      }
'
if (Visited.insert(D)) {
   WorkList.push_back(D);
}

LGTM!

Nick

> On Tue, Nov 13, 2012 at 7:47 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>> 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.
>>
>> +/// 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.
>
> Done.
>
>> +      if (C->isThreadDependent())
>>
>> Two things. Firstly, I'm worried about stack depth, so I'd rather this
>> function maintained its own worklist.
>
> Done.
>
>> 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.
>
> OK. I won't worry about this for now, then.
>
>> @@ -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!)
>
> Indeed.
>
>>> 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.
>
> But memdep, gvn and dse are all function passes, right? I think
> (please correct me if I'm wrong here) it's safe to assume that a TLS
> variable has a constant address within a function, so this kind of
> problem only arises for inter-procedural optimizations/analyses.

You're right. Creating a thread, unlike forking, happens to take a 
function address for the thread_start. I don't know of any OS where you 
can create a thread like you fork.

> I don't understand your comment about readonly "create a
> thread"-functions. What does that imply?
>
>> I think this is just another reason we need to kill ConstantExpr, and forbid
>> expressions involving TLS to be constant.
>
> Has there been previous talk about killing ConstantExpr?

Yes, PR10368.

> Yes, the fundamental problem here is really that a TLS variable is a
> GlobalVariable, and therefore also Constant -- and it turns out that
> we can't assume that it's constant between functions :/
>
> Thanks,
> Hans