On Wed, Oct 7, 2015 at 1:48 PM, Duncan P. N. Exon Smith < dexonsm...@apple.com> wrote:
> > > On 2015-Oct-07, at 11:35, Akira Hatanaka <ahata...@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()) { > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits