[PATCH] Add NoDuplicate attribute to clang (2nd try)
Aaron Ballman
aaron at aaronballman.com
Sat Feb 22 09:05:47 PST 2014
Thanks! I've applied your changes in r201941. I did have to change the
codegen test so that it had a target triple, otherwise it was failing
for me on Windows due to name mangling differences.
~Aaron
On Fri, Feb 21, 2014 at 8:52 PM, Marcello Maggioni <hayarms at gmail.com> wrote:
> 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
More information about the cfe-commits
mailing list