[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