[PATCH] D81605: IR: Add missing GlobalAlias copying of ThreadLocalMode attribute

[Itay Bookstein via Phabricator via llvm-commits]

ibookstein added a comment.

Added inline reply about IRMover and where the copying belongs.

I think it least bug-prone to push the ThreadLocalMode copying from GlobalVariable::copyAttributesFrom down to GlobalValue::copyAttributesFrom. 
Values in the hierarchy for which the attribute is not relevant will ignore it. What do you think?

Regarding patch applicability to master:
You're right, I prepared the patch on an LLVM-9-derived branch. Will fix according to the decided-upon changes, and add the relevant test.



================
Comment at: llvm/include/llvm/IR/GlobalAlias.h:63
     GlobalValue::copyAttributesFrom(Src);
+    setThreadLocalMode(Src->getThreadLocalMode());
   }
----------------
tejohnson wrote:
> It seems like this belongs in GlobalValue::copyAttributesFrom, since it is a property of the GlobalValue.
> 
> Interestingly I do see code in the IRMover (used when linking IR in LTO) that copies it over (in llvm/lib/Linker/IRMover.cpp). Are we missing places there? In any case, it seems reasonable to add this into GlobalValue::copyAttributesFrom.
Regarding where it belongs:
At first I put it in GlobalValue::copyAttributesFrom, but then I saw that that exact line already was in [[ https://github.com/llvm/llvm-project/blob/2e009dbcb3e373a59e6e84dce6d51ae8a29f60a5/llvm/lib/IR/Globals.cpp#L422 | GlobalVariable::copyAttributesFrom ]]. So I'd assumed that it's intentionally put only in derived classes for which that attribute is relevant (i.e. GlobalVariable and GlobalAlias, but not GlobalIFunc and Function). I see no harm in putting it in the base class where it's defined, but I defer to your judgement.

Regarding the code in IRMover:
I do indeed see code that passes the source value's ThreadLocalMode, but only to GlobalVariable constructors. It seems there's a slight non-uniformity of interface between the constructors in that respect, as the GlobalAlias constructors (or rather, static factory methods) don't expose a way to pass the ThreadLocalMode at construction. [[ https://github.com/llvm/llvm-project/blob/c5bbdea9e120dda9632a71708a852cb32317ed92/llvm/lib/Linker/IRMover.cpp#L678 | The code ]] in IRMover that creates GlobalAliases does not touch the ThreadLocalMode, and then static-dispatches to GlobalIndirectSymbol::copyAttributesFrom, which in turn just forwards the call to GlobalValue::copyAttributesFrom (where there's no handling for the ThreadLocalMode).



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81605/new/

https://reviews.llvm.org/D81605