[PATCH] D44185: [Coroutines] Avoid assert splitting hidden coros

[Brian Gesiak via Phabricator via llvm-commits]

modocache added inline comments.


================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:266
 
   CloneFunctionInto(NewF, &F, VMap, /*ModuleLevelChanges=*/true, Returns);
   NewF->setDSOLocal(true);
----------------
rnk wrote:
> modocache wrote:
> > @rnk This function, `CloneFunctionInto`, calls `Function::copyAttributesFrom`, which in turn calls `GlobalValue::copyAttributesFrom` and finally `GlobalValue::setVisibility`, which triggers the assert when the original function `F` has hidden linkage.
> > 
> > > I think the right fix is to leave these internal and to avoid copying the visibility.
> > 
> > That sounds like a good idea to me, too. It might be a little tricky because the copying occurs several layers deep.
> > 
> > Perhaps I could add a `Function::copyAttributesFrom` overload that also takes, as an argument, a list of attributes to //not// copy? I could then prevent the visibility attribute from being copied here, in [[ https://github.com/llvm-mirror/llvm/blob/80548aa3fb95eba87b03ca058b598699f7d98143/lib/Transforms/Utils/CloneFunction.cpp#L106-L110 | lib/Transforms/Utils/CloneFunction.cpp ]]:
> > 
> > ```
> > void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
> >                              ValueToValueMapTy &VMap,
> >                              bool ModuleLevelChanges,
> >                              SmallVectorImpl<ReturnInst*> &Returns,
> >                              const char *NameSuffix, ClonedCodeInfo *CodeInfo,
> >                              ValueMapTypeRemapper *TypeMapper,
> >                              ValueMaterializer *Materializer) {
> >   // ...
> > 
> >   // Copy all attributes other than those stored in the AttributeList.  We need
> >   // to remap the parameter indices of the AttributeList.
> >   AttributeList NewAttrs = NewFunc->getAttributes();
> >   NewFunc->copyAttributesFrom(OldFunc);
> >   NewFunc->setAttributes(NewAttrs);
> > 
> >   // ...
> > }
> > ```
> > 
> > It seems like it's the intention of this code to only copy attributes that aren't set on the new function, so an overload may express this intent even better. And preventing the visibility from being copied should prevent this assert from being hit as well. What do you think? Does that sound like a reasonable approach?
> I would suggest creating the function with external linkage, and then down here after CloneFunctionInfo call setLinkage, which will reset the visibility and DSO local bits.
Good thinking! Thanks, I'll do this.


Repository:
  rL LLVM

https://reviews.llvm.org/D44185