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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/IR/GlobalAlias.h:63
     GlobalValue::copyAttributesFrom(Src);
+    setThreadLocalMode(Src->getThreadLocalMode());
   }
----------------
ibookstein wrote:
> 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).
> 
I agree that since this property is defined on the GlobalValue it makes most sense to copy it in the GlobalValue::copyAttributesFrom. I am not an expert on this particular attribute though. I discovered that Hans Wennborg added it some years ago (circa 2012-2014!). Added him to the review here to comment. I'm guessing that it was meant to apply to variables but leaving it off of aliases was an oversight?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81605