[PATCH] D13525: [CodeGen] Attach function attributes to functions created in CGBlocks.cpp.

[Duncan P. N. Exon Smith via cfe-commits]

> On 2015-Oct-07, at 11:35, Akira Hatanaka <ahatanak at gmail.com> wrote:
> 
> ahatanak created this revision.
> ahatanak added reviewers: dexonsmith, echristo.
> ahatanak added a subscriber: cfe-commits.
> 
> This patch makes changes to attach function attributes to the following functions created in CGBlocks.cpp:
> 
> __copy_helper_block_
> __destroy_helper_block_
> __Block_byref_object_copy_
> __Block_byref_object_dispose_
> 
> There are other places in clang where function attributes are not attached to functions. I plan to follow up with patches that fix those places too.
> 
> http://reviews.llvm.org/D13525
> 
> Files:
>  lib/CodeGen/CGBlocks.cpp
>  lib/CodeGen/CodeGenModule.cpp
>  lib/CodeGen/TargetInfo.cpp
>  test/CodeGenObjC/arc-blocks.m
> 
> <D13525.36773.patch>

> Index: lib/CodeGen/CodeGenModule.cpp
> ===================================================================
> --- lib/CodeGen/CodeGenModule.cpp
> +++ lib/CodeGen/CodeGenModule.cpp
> @@ -786,29 +786,32 @@
>    if (!hasUnwindExceptions(LangOpts))
>      B.addAttribute(llvm::Attribute::NoUnwind);
>  
> -  if (D->hasAttr<NakedAttr>()) {
> -    // Naked implies noinline: we should not be inlining such functions.
> -    B.addAttribute(llvm::Attribute::Naked);
> -    B.addAttribute(llvm::Attribute::NoInline);
> -  } else if (D->hasAttr<NoDuplicateAttr>()) {
> -    B.addAttribute(llvm::Attribute::NoDuplicate);
> -  } else if (D->hasAttr<NoInlineAttr>()) {
> -    B.addAttribute(llvm::Attribute::NoInline);
> -  } else if (D->hasAttr<AlwaysInlineAttr>() &&
> -             !F->getAttributes().hasAttribute(llvm::AttributeSet::FunctionIndex,
> -                                              llvm::Attribute::NoInline)) {
> -    // (noinline wins over always_inline, and we can't specify both in IR)
> -    B.addAttribute(llvm::Attribute::AlwaysInline);
> -  }
> -
> -  if (D->hasAttr<ColdAttr>()) {
> -    if (!D->hasAttr<OptimizeNoneAttr>())
> -      B.addAttribute(llvm::Attribute::OptimizeForSize);
> -    B.addAttribute(llvm::Attribute::Cold);
> -  }
> -
> -  if (D->hasAttr<MinSizeAttr>())
> -    B.addAttribute(llvm::Attribute::MinSize);
> +  if (D) {

I wonder, is there a way to reorder this function so that you can use an
early return here?  The extra indentation (and resulting noise) is
unfortunate.

Really, the way attributes are added should be refactored.  I spent a few
days trying to do that in the spring but (obviously) never got to
anything committable.  If you have any ideas for ways to clean this up
while you're in here, please "fix" it.

Anyway, LGTM after a prep NFC commit to avoid the extra indentation.

> +    if (D->hasAttr<NakedAttr>()) {
> +      // Naked implies noinline: we should not be inlining such functions.
> +      B.addAttribute(llvm::Attribute::Naked);
> +      B.addAttribute(llvm::Attribute::NoInline);
> +    } else if (D->hasAttr<NoDuplicateAttr>()) {
> +      B.addAttribute(llvm::Attribute::NoDuplicate);
> +    } else if (D->hasAttr<NoInlineAttr>()) {
> +      B.addAttribute(llvm::Attribute::NoInline);
> +    } else if (D->hasAttr<AlwaysInlineAttr>() &&
> +               !F->getAttributes().hasAttribute(
> +                   llvm::AttributeSet::FunctionIndex,
> +                   llvm::Attribute::NoInline)) {
> +      // (noinline wins over always_inline, and we can't specify both in IR)
> +      B.addAttribute(llvm::Attribute::AlwaysInline);
> +    }
> +
> +    if (D->hasAttr<ColdAttr>()) {
> +      if (!D->hasAttr<OptimizeNoneAttr>())
> +        B.addAttribute(llvm::Attribute::OptimizeForSize);
> +      B.addAttribute(llvm::Attribute::Cold);
> +    }
> +
> +    if (D->hasAttr<MinSizeAttr>())
> +      B.addAttribute(llvm::Attribute::MinSize);
> +  }
>  
>    if (LangOpts.getStackProtector() == LangOptions::SSPOn)
>      B.addAttribute(llvm::Attribute::StackProtect);
> @@ -821,6 +824,9 @@
>                     llvm::AttributeSet::get(
>                         F->getContext(), llvm::AttributeSet::FunctionIndex, B));
>  
> +  if (!D)
> +    return;
> +
>    if (D->hasAttr<OptimizeNoneAttr>()) {
>      // OptimizeNone implies noinline; we should not be inlining such functions.
>      F->addFnAttr(llvm::Attribute::OptimizeNone);
> @@ -859,12 +865,12 @@
>  
>  void CodeGenModule::SetCommonAttributes(const Decl *D,
>                                          llvm::GlobalValue *GV) {
> -  if (const auto *ND = dyn_cast<NamedDecl>(D))
> +  if (const auto *ND = dyn_cast_or_null<NamedDecl>(D))
>      setGlobalVisibility(GV, ND);
>    else
>      GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
>  
> -  if (D->hasAttr<UsedAttr>())
> +  if (D && D->hasAttr<UsedAttr>())
>      addUsedGlobal(GV);
>  }
>  
> @@ -882,8 +888,9 @@
>                                            llvm::GlobalObject *GO) {
>    SetCommonAttributes(D, GO);
>  
> -  if (const SectionAttr *SA = D->getAttr<SectionAttr>())
> -    GO->setSection(SA->getName());
> +  if (D)
> +    if (const SectionAttr *SA = D->getAttr<SectionAttr>())
> +      GO->setSection(SA->getName());
>  
>    getTargetCodeGenInfo().setTargetAttributes(D, GO, *this);
>  }
> Index: lib/CodeGen/CGBlocks.cpp
> ===================================================================
> --- lib/CodeGen/CGBlocks.cpp
> +++ lib/CodeGen/CGBlocks.cpp
> @@ -1345,6 +1345,9 @@
>                                            nullptr, SC_Static,
>                                            false,
>                                            false);
> +
> +  CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);
> +
>    auto NL = ApplyDebugLocation::CreateEmpty(*this);
>    StartFunction(FD, C.VoidTy, Fn, FI, args);
>    // Create a scope with an artificial location for the body of this function.
> @@ -1516,6 +1519,9 @@
>                                            SourceLocation(), II, C.VoidTy,
>                                            nullptr, SC_Static,
>                                            false, false);
> +
> +  CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);
> +
>    // Create a scope with an artificial location for the body of this function.
>    auto NL = ApplyDebugLocation::CreateEmpty(*this);
>    StartFunction(FD, C.VoidTy, Fn, FI, args);
> @@ -1798,6 +1804,8 @@
>                                            SC_Static,
>                                            false, false);
>  
> +  CGF.CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);
> +
>    CGF.StartFunction(FD, R, Fn, FI, args);
>  
>    if (generator.needsCopy()) {
> @@ -1869,6 +1877,9 @@
>                                            SourceLocation(), II, R, nullptr,
>                                            SC_Static,
>                                            false, false);
> +
> +  CGF.CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);
> +
>    CGF.StartFunction(FD, R, Fn, FI, args);
>  
>    if (generator.needsDispose()) {
>