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 <[email protected]> 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 <[email protected]>: > >> 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 <[email protected]> >> 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 >> > [email protected] >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > > > > Cheers, > Marcello _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
