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