<div dir="ltr">Hi Aaron,<div class="gmail_extra">thanks for your comments! I addressed your concerns in these 2 new patches. The answers are inline<br><br><div class="gmail_quote">2014-02-21 23:04 GMT+00:00 Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">A few mostly minor comments below:<br>
<br>
> diff --git a/docs/AttributeReference.rst b/docs/AttributeReference.rst<br>
> index 1d41cb3..3d718a4 100644<br>
> --- a/docs/AttributeReference.rst<br>
> +++ b/docs/AttributeReference.rst<br>
> @@ -558,6 +558,50 @@ caveats to this use of name mangling:<br>
><br>
> Query for this feature with ``__has_extension(attribute_overloadable)``.<br>
><br>
> +noduplicate<br>
> +-----------<br>
> +.. csv-table:: Supported Syntaxes<br>
> + :header: "GNU", "C++11", "__declspec", "Keyword"<br>
> +<br>
> + "X","X","",""<br>
> +<br>
> +The ``noduplicate`` attribute can be placed on function declarations to control<br>
> +which overload whether function calls to this function can be duplicated<br>
> +or not as a result of optimizations. This is required for the implementation<br>
> +of functions with certain special requirements, like the OpenCL "barrier",<br>
> +function that, depending on the hardware, might require to be run concurrently<br>
> +by all the threads that are currently executing in lockstep on the hardware.<br>
> +For example this attribute applied on the function "nodupfunc"<br>
> +avoids that this code:<br>
> +<br>
> +.. code-block:: c<br>
> +<br>
> + void nodupfunc() __attribute__((noduplicate));<br>
> + // Setting it as a C++11 attribute is also valid<br>
> + // void nodupfunc() [[clang::noduplicate]];<br>
> + void foo();<br>
> + void bar();<br>
> +<br>
> + nodupfunc();<br>
> + if (a > n) {<br>
> + foo();<br>
> + } else {<br>
> + bar();<br>
> + }<br>
> +<br>
> +gets possibly modified by some optimization into code similar to this:<br>
> +<br>
> +.. code-block:: c<br>
> +<br>
> + if (a > n) {<br>
> + nodupfunc();<br>
> + foo();<br>
> + } else {<br>
> + nodupfunc();<br>
> + bar();<br>
> + }<br>
> +<br>
> +where the barrier call is duplicated and sinked into the two branches of the condition.<br>
><br>
> Variable Attributes<br>
> ===================<br>
> diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td<br>
> index 3b70c4c..1368812 100644<br>
> --- a/include/clang/Basic/Attr.td<br>
> +++ b/include/clang/Basic/Attr.td<br>
> @@ -803,6 +803,12 @@ def NoDebug : InheritableAttr {<br>
> let Documentation = [Undocumented];<br>
> }<br>
><br>
> +def NoDuplicate : InheritableAttr {<br>
> + let Spellings = [GNU<"noduplicate">, CXX11<"clang", "noduplicate">];<br>
> + let Subjects = SubjectList<[Function]>;<br>
> + let Documentation = [NoDuplicateDocs];<br>
> +}<br>
> +<br>
> def NoInline : InheritableAttr {<br>
> let Spellings = [GCC<"noinline">, Declspec<"noinline">];<br>
> let Subjects = SubjectList<[Function]>;<br>
> diff --git a/include/clang/Basic/AttrDocs.td b/include/clang/Basic/AttrDocs.td<br>
> index a2476e4..d60de82 100644<br>
> --- a/include/clang/Basic/AttrDocs.td<br>
> +++ b/include/clang/Basic/AttrDocs.td<br>
> @@ -268,6 +268,49 @@ Query for this feature with ``__has_attribute(objc_method_family)``.<br>
> }];<br>
> }<br>
><br>
> +def NoDuplicateDocs : Documentation {<br>
> + let Category = DocCatFunction;<br>
> + let Content = [{<br>
> +The ``noduplicate`` attribute can be placed on function declarations to control<br>
> +which overload whether function calls to this function can be duplicated<br>
<br>
"which overload whether" reads awkwardly.<br>
<br></blockquote><div><br></div><div>Hmm , I think that because I borrowed the template for the docs from the "overloadable" attribute that "which overload" expression might have sneaked into the final version of the NoDuplicateDocs ...</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +or not as a result of optimizations. This is required for the implementation<br>
> +of functions with certain special requirements, like the OpenCL "barrier",<br>
> +function that, depending on the hardware, might require to be run concurrently<br>
> +by all the threads that are currently executing in lockstep on the hardware.<br>
> +For example this attribute applied on the function "nodupfunc"<br>
> +avoids that this code:<br>
> +<br>
> +.. code-block:: c<br>
> +<br>
> + void nodupfunc() __attribute__((noduplicate));<br>
> + // Setting it as a C++11 attribute is also valid<br>
> + // void nodupfunc() [[clang::noduplicate]];<br>
> + void foo();<br>
> + void bar();<br>
> +<br>
> + nodupfunc();<br>
> + if (a > n) {<br>
> + foo();<br>
> + } else {<br>
> + bar();<br>
> + }<br>
> +<br>
> +gets possibly modified by some optimization into code similar to this:<br>
> +<br>
> +.. code-block:: c<br>
> +<br>
> + if (a > n) {<br>
> + nodupfunc();<br>
> + foo();<br>
> + } else {<br>
> + nodupfunc();<br>
> + bar();<br>
> + }<br>
> +<br>
> +where the barrier call is duplicated and sinked into the two branches of the condition.<br>
<br>
"sunk" instead of "sinked"?<br>
<br></blockquote><div><br></div><div>Ehmmmm, no excuse for this one :D</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + }];<br>
> +}<br>
> +<br>
> def ObjCRequiresSuperDocs : Documentation {<br>
> let Category = DocCatFunction;<br>
> let Content = [{<br>
> @@ -811,4 +854,4 @@ Clang implements two kinds of checks with this attribute.<br>
> the corresponding arguments are annotated. If the arguments are<br>
> incorrect, the caller of ``foo`` will receive a warning.<br>
> }];<br>
> -}<br>
> \ No newline at end of file<br>
> +}<br>
> diff --git a/lib/CodeGen/CGCall.cpp b/lib/CodeGen/CGCall.cpp<br>
> index a9caa88..a21e478 100644<br>
> --- a/lib/CodeGen/CGCall.cpp<br>
> +++ b/lib/CodeGen/CGCall.cpp<br>
> @@ -1052,6 +1052,8 @@ void CodeGenModule::ConstructAttributeList(const CGFunctionInfo &FI,<br>
> FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);<br>
> if (TargetDecl->hasAttr<NoReturnAttr>())<br>
> FuncAttrs.addAttribute(llvm::Attribute::NoReturn);<br>
> + if (TargetDecl->hasAttr<NoDuplicateAttr>())<br>
> + FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);<br>
><br>
> if (const FunctionDecl *Fn = dyn_cast<FunctionDecl>(TargetDecl)) {<br>
> const FunctionProtoType *FPT = Fn->getType()->getAs<FunctionProtoType>();<br>
> diff --git a/lib/CodeGen/CodeGenModule.cpp b/lib/CodeGen/CodeGenModule.cpp<br>
> index bbf5a73..e6798e4 100644<br>
> --- a/lib/CodeGen/CodeGenModule.cpp<br>
> +++ b/lib/CodeGen/CodeGenModule.cpp<br>
> @@ -631,6 +631,8 @@ void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,<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>
> diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp<br>
> index ef19a0b..4557870 100644<br>
> --- a/lib/Sema/SemaDeclAttr.cpp<br>
> +++ b/lib/Sema/SemaDeclAttr.cpp<br>
> @@ -3004,6 +3004,18 @@ static void handleNoDebugAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
> Attr.getAttributeSpellingListIndex()));<br>
> }<br>
><br>
> +static void handleNoDuplicateAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
> + if (!isa<FunctionDecl>(D)) {<br>
> + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)<br>
> + << Attr.getName() << ExpectedFunction;<br>
> + return;<br>
> + }<br>
> +<br>
> + D->addAttr(::new (S.Context)<br>
> + NoDuplicateAttr(Attr.getRange(), S.Context,<br>
> + Attr.getAttributeSpellingListIndex()));<br>
> +}<br>
<br>
This can all be removed.<br>
<br>
> +<br>
> static void handleGlobalAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
> FunctionDecl *FD = cast<FunctionDecl>(D);<br>
> if (!FD->getReturnType()->isVoidType()) {<br>
> @@ -4176,6 +4188,7 @@ static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,<br>
> handleSimpleAttribute<PureAttr>(S, D, Attr); break;<br>
> case AttributeList::AT_Cleanup: handleCleanupAttr (S, D, Attr); break;<br>
> case AttributeList::AT_NoDebug: handleNoDebugAttr (S, D, Attr); break;<br>
> + case AttributeList::AT_NoDuplicate: handleNoDuplicateAttr (S, D, Attr); break;<br>
<br>
handleNoDuplicateAttr can be replaced wtih<br>
handleSimpleAttribute<NoDuplicateAttr>(S, D, Attr);<br>
<br>
The common attribute checking functionality already handles ensuring<br>
that subjects are checked, which is all your custom handler was doing<br>
(which is why it could be removed).<br>
<br></blockquote><div><br></div><div>Perfect! Thanks for this! Makes the patch simpler</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> case AttributeList::AT_NoInline:<br>
> handleSimpleAttribute<NoInlineAttr>(S, D, Attr); break;<br>
> case AttributeList::AT_NoInstrumentFunction: // Interacts with -pg.<br>
><br>
> diff --git a/test/CodeGen/noduplicate-cxx11-test.cpp b/test/CodeGen/noduplicate-cxx11-test.cpp<br>
> new file mode 100644<br>
> index 0000000..09e3801<br>
> --- /dev/null<br>
> +++ b/test/CodeGen/noduplicate-cxx11-test.cpp<br>
> @@ -0,0 +1,20 @@<br>
> +// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - | FileCheck %s<br>
> +<br>
> +// This was a problem in Sema, but only shows up as noinline missing<br>
> +// in CodeGen.<br>
> +<br>
> +// CHECK: define i32 @_Z15noduplicatedfuni(i32 %a) [[NI:#[0-9]+]]<br>
> +<br>
> +int noduplicatedfun [[clang::noduplicate]] (int a) {<br>
> +<br>
> + return a+1;<br>
> +<br>
> +}<br>
> +<br>
> +int main() {<br>
> +<br>
> + return noduplicatedfun(5);<br>
> +<br>
> +}<br>
> +<br>
> +// CHECK: attributes [[NI]] = { noduplicate nounwind{{.*}} }<br>
> diff --git a/test/CodeGen/noduplicate-test.cpp b/test/CodeGen/noduplicate-test.cpp<br>
> new file mode 100644<br>
> index 0000000..86413a8<br>
> --- /dev/null<br>
> +++ b/test/CodeGen/noduplicate-test.cpp<br>
<br>
I don't think this second codegen test is needed. It's identical to<br>
the first one aside from the attribute syntax used.<br>
<br></blockquote><div>Ok, makes sense. Originally I wanted to try if both of them worked correctly, but if you think one is enough then I'll remove the second one and keep the CXX11 one :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> @@ -0,0 +1,20 @@<br>
> +// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s<br>
> +<br>
> +// This was a problem in Sema, but only shows up as noinline missing<br>
> +// in CodeGen.<br>
> +<br>
> +// CHECK: define i32 @_Z15noduplicatedfuni(i32 %a) [[NI:#[0-9]+]]<br>
> +<br>
> +__attribute__((noduplicate)) int noduplicatedfun(int a) {<br>
> +<br>
> + return a+1;<br>
> +<br>
> +}<br>
> +<br>
> +int main() {<br>
> +<br>
> + return noduplicatedfun(5);<br>
> +<br>
> +}<br>
> +<br>
> +// CHECK: attributes [[NI]] = { noduplicate nounwind{{.*}} }<br>
> diff --git a/test/Sema/attr-noduplicate.c b/test/Sema/attr-noduplicate.c<br>
> new file mode 100644<br>
> index 0000000..2a77de5<br>
> --- /dev/null<br>
> +++ b/test/Sema/attr-noduplicate.c<br>
> @@ -0,0 +1,8 @@<br>
> +// RUN: %clang_cc1 %s -verify -fsyntax-only<br>
> +<br>
> +int a __attribute__((noduplicate)); // expected-warning {{'noduplicate' attribute only applies to functions}}<br>
> +<br>
> +void t1() __attribute__((noduplicate));<br>
> +<br>
> +void t2() __attribute__((noduplicate(2))); // expected-error {{'noduplicate' attribute takes no arguments}}<br>
> +<br>
><br>
<br>
~Aaron<br>
<div><div class="h5"><br>
On Fri, Feb 21, 2014 at 3:31 PM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com">hayarms@gmail.com</a>> wrote:<br>
> Hi everybody,<br>
><br>
> back in November I made a patch that added the noduplicate attribute to<br>
> Clang (original discussion here<br>
> <a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131118/093647.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131118/093647.html</a><br>
> ). After passing multiple reviews in the end I went in holiday and the patch<br>
> got lost and not committed :P<br>
><br>
> So here I try again to get this patch committed. The patch is needed by some<br>
> OpenCL implementers , because it permits to mark functions as "Not<br>
> duplicable" by optimization passes. (in the sense that function calls to the<br>
> function cannot be duplicated).<br>
> Some functions require that optimization that convert code like this:<br>
><br>
> nodupfunc();<br>
> if (a < n) {<br>
> foo();<br>
> } else {<br>
> bar();<br>
> }<br>
><br>
> into this<br>
><br>
> if (a < n) {<br>
> nodupfunc();<br>
> foo();<br>
> } else {<br>
> nodupfunc();<br>
> bar();<br>
> }<br>
><br>
> do not consider the function call to "nodupfunc" for optimization. If the<br>
> "nodupfunc" would have been marked as "NoDuplicate" the optimization<br>
> wouldn't have performed the sinking and duplication of the function call.<br>
> Typical examples of functions that requires this are some implementations of<br>
> the OpenCL barrier() function.<br>
><br>
> That said, the patch is basically the same to the last one I posted back in<br>
> november, updated to work with the latest clang code and I also added<br>
> documentation for the new attribute as suggested by Richard Smith in one of<br>
> the last messages.<br>
><br>
> The patch is split in two. One contains the code and one the tests for the<br>
> new attribute.<br>
><br>
> If somebody can review it I'd appreciate :) I also don't have commit access,<br>
> so I would need someone to commit it for me if it is fine.<br>
><br>
> Cheers,<br>
> Marcello<br>
><br>
</div></div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
</blockquote></div><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Cheers,</div><div class="gmail_extra">Marcello</div></div>