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

Akira Hatanaka via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 7 15:06:51 PDT 2015


On Wed, Oct 7, 2015 at 1:48 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > 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.
>
>
I think we would need to call F->addAttributes() in two places (but not
twice) if we want to do early return. Would that look better?


> 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.
>
>
>From an efficiency point of view, I think we should stop calling
Function::addFnAttr and removeFnAttr to add or remove a single attribute.
Instead, AttrBuilder should be used and Function::addAttributes should be
called only when we the builder has a complete set of attributes. Also, I
think we need a member function of Function or AttributeSet that replaces
the attribute set at a certain index.


> 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()) {
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151007/dbcdf588/attachment-0001.html>


More information about the cfe-commits mailing list