[PATCH] Add NoDuplicate attribute to clang (2nd try)
Aaron Ballman
aaron at aaronballman.com
Fri Feb 21 15:04:53 PST 2014
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.
> +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"?
> + }];
> +}
> +
> 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).
> 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.
> @@ -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
>
More information about the cfe-commits
mailing list