<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Oct 7, 2015 at 1:48 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br>
> On 2015-Oct-07, at 11:35, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:<br>
><br>
> ahatanak created this revision.<br>
> ahatanak added reviewers: dexonsmith, echristo.<br>
> ahatanak added a subscriber: cfe-commits.<br>
><br>
> This patch makes changes to attach function attributes to the following functions created in CGBlocks.cpp:<br>
><br>
> __copy_helper_block_<br>
> __destroy_helper_block_<br>
> __Block_byref_object_copy_<br>
> __Block_byref_object_dispose_<br>
><br>
> 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.<br>
><br>
> <a href="http://reviews.llvm.org/D13525" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13525</a><br>
><br>
> Files:<br>
> lib/CodeGen/CGBlocks.cpp<br>
> lib/CodeGen/CodeGenModule.cpp<br>
> lib/CodeGen/TargetInfo.cpp<br>
> test/CodeGenObjC/arc-blocks.m<br>
><br>
</div></div>> <D13525.36773.patch><br>
<br>
> Index: lib/CodeGen/CodeGenModule.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CodeGenModule.cpp<br>
> +++ lib/CodeGen/CodeGenModule.cpp<br>
> @@ -786,29 +786,32 @@<br>
> if (!hasUnwindExceptions(LangOpts))<br>
> B.addAttribute(llvm::Attribute::NoUnwind);<br>
><br>
> - if (D->hasAttr<NakedAttr>()) {<br>
> - // Naked implies noinline: we should not be inlining such functions.<br>
> - B.addAttribute(llvm::Attribute::Naked);<br>
> - B.addAttribute(llvm::Attribute::NoInline);<br>
> - } else if (D->hasAttr<NoDuplicateAttr>()) {<br>
> - B.addAttribute(llvm::Attribute::NoDuplicate);<br>
> - } else if (D->hasAttr<NoInlineAttr>()) {<br>
> - B.addAttribute(llvm::Attribute::NoInline);<br>
> - } else if (D->hasAttr<AlwaysInlineAttr>() &&<br>
> - !F->getAttributes().hasAttribute(llvm::AttributeSet::FunctionIndex,<br>
> - llvm::Attribute::NoInline)) {<br>
> - // (noinline wins over always_inline, and we can't specify both in IR)<br>
> - B.addAttribute(llvm::Attribute::AlwaysInline);<br>
> - }<br>
> -<br>
> - if (D->hasAttr<ColdAttr>()) {<br>
> - if (!D->hasAttr<OptimizeNoneAttr>())<br>
> - B.addAttribute(llvm::Attribute::OptimizeForSize);<br>
> - B.addAttribute(llvm::Attribute::Cold);<br>
> - }<br>
> -<br>
> - if (D->hasAttr<MinSizeAttr>())<br>
> - B.addAttribute(llvm::Attribute::MinSize);<br>
> + if (D) {<br>
<br>
I wonder, is there a way to reorder this function so that you can use an<br>
early return here? The extra indentation (and resulting noise) is<br>
unfortunate.<br>
<br></blockquote><div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Really, the way attributes are added should be refactored. I spent a few<br>
days trying to do that in the spring but (obviously) never got to<br>
anything committable. If you have any ideas for ways to clean this up<br>
while you're in here, please "fix" it.<br>
<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Anyway, LGTM after a prep NFC commit to avoid the extra indentation.<br>
<br>
> + if (D->hasAttr<NakedAttr>()) {<br>
> + // Naked implies noinline: we should not be inlining such functions.<br>
> + B.addAttribute(llvm::Attribute::Naked);<br>
> + B.addAttribute(llvm::Attribute::NoInline);<br>
> + } else if (D->hasAttr<NoDuplicateAttr>()) {<br>
> + B.addAttribute(llvm::Attribute::NoDuplicate);<br>
> + } else if (D->hasAttr<NoInlineAttr>()) {<br>
> + B.addAttribute(llvm::Attribute::NoInline);<br>
> + } else if (D->hasAttr<AlwaysInlineAttr>() &&<br>
> + !F->getAttributes().hasAttribute(<br>
> + llvm::AttributeSet::FunctionIndex,<br>
> + llvm::Attribute::NoInline)) {<br>
> + // (noinline wins over always_inline, and we can't specify both in IR)<br>
> + B.addAttribute(llvm::Attribute::AlwaysInline);<br>
> + }<br>
> +<br>
> + if (D->hasAttr<ColdAttr>()) {<br>
> + if (!D->hasAttr<OptimizeNoneAttr>())<br>
> + B.addAttribute(llvm::Attribute::OptimizeForSize);<br>
> + B.addAttribute(llvm::Attribute::Cold);<br>
> + }<br>
> +<br>
> + if (D->hasAttr<MinSizeAttr>())<br>
> + B.addAttribute(llvm::Attribute::MinSize);<br>
> + }<br>
><br>
> if (LangOpts.getStackProtector() == LangOptions::SSPOn)<br>
> B.addAttribute(llvm::Attribute::StackProtect);<br>
> @@ -821,6 +824,9 @@<br>
> llvm::AttributeSet::get(<br>
> F->getContext(), llvm::AttributeSet::FunctionIndex, B));<br>
><br>
> + if (!D)<br>
> + return;<br>
> +<br>
> if (D->hasAttr<OptimizeNoneAttr>()) {<br>
> // OptimizeNone implies noinline; we should not be inlining such functions.<br>
> F->addFnAttr(llvm::Attribute::OptimizeNone);<br>
> @@ -859,12 +865,12 @@<br>
><br>
> void CodeGenModule::SetCommonAttributes(const Decl *D,<br>
> llvm::GlobalValue *GV) {<br>
> - if (const auto *ND = dyn_cast<NamedDecl>(D))<br>
> + if (const auto *ND = dyn_cast_or_null<NamedDecl>(D))<br>
> setGlobalVisibility(GV, ND);<br>
> else<br>
> GV->setVisibility(llvm::GlobalValue::DefaultVisibility);<br>
><br>
> - if (D->hasAttr<UsedAttr>())<br>
> + if (D && D->hasAttr<UsedAttr>())<br>
> addUsedGlobal(GV);<br>
> }<br>
><br>
> @@ -882,8 +888,9 @@<br>
> llvm::GlobalObject *GO) {<br>
> SetCommonAttributes(D, GO);<br>
><br>
> - if (const SectionAttr *SA = D->getAttr<SectionAttr>())<br>
> - GO->setSection(SA->getName());<br>
> + if (D)<br>
> + if (const SectionAttr *SA = D->getAttr<SectionAttr>())<br>
> + GO->setSection(SA->getName());<br>
><br>
> getTargetCodeGenInfo().setTargetAttributes(D, GO, *this);<br>
> }<br>
> Index: lib/CodeGen/CGBlocks.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CGBlocks.cpp<br>
> +++ lib/CodeGen/CGBlocks.cpp<br>
> @@ -1345,6 +1345,9 @@<br>
> nullptr, SC_Static,<br>
> false,<br>
> false);<br>
> +<br>
> + CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);<br>
> +<br>
> auto NL = ApplyDebugLocation::CreateEmpty(*this);<br>
> StartFunction(FD, C.VoidTy, Fn, FI, args);<br>
> // Create a scope with an artificial location for the body of this function.<br>
> @@ -1516,6 +1519,9 @@<br>
> SourceLocation(), II, C.VoidTy,<br>
> nullptr, SC_Static,<br>
> false, false);<br>
> +<br>
> + CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);<br>
> +<br>
> // Create a scope with an artificial location for the body of this function.<br>
> auto NL = ApplyDebugLocation::CreateEmpty(*this);<br>
> StartFunction(FD, C.VoidTy, Fn, FI, args);<br>
> @@ -1798,6 +1804,8 @@<br>
> SC_Static,<br>
> false, false);<br>
><br>
> + CGF.CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);<br>
> +<br>
> CGF.StartFunction(FD, R, Fn, FI, args);<br>
><br>
> if (generator.needsCopy()) {<br>
> @@ -1869,6 +1877,9 @@<br>
> SourceLocation(), II, R, nullptr,<br>
> SC_Static,<br>
> false, false);<br>
> +<br>
> + CGF.CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);<br>
> +<br>
> CGF.StartFunction(FD, R, Fn, FI, args);<br>
><br>
> if (generator.needsDispose()) {<br>
><br>
<br>
</blockquote></div><br></div></div>