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

[Hans Wennborg]

Thanks for the review, Nick! Attaching new patch addressing your comments.

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.

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, 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr14309_tls_globalopt3.diff
Type: application/octet-stream
Size: 4714 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121114/aa4f5d6f/attachment.obj>