[PATCH] D29599: Clang Changes for alloc_align
This revision was automatically updated to reflect the committed changes.
Closed by commit rL299117: Clang changes for alloc_align attribute (authored
by erichkeane).
Changed prior to commit:
https://reviews.llvm.org/D29599?vs=93537=93549#toc
Repository:
rL LLVM
https://reviews.llvm.org/D29599
Files:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
cfe/trunk/test/CodeGen/alloc-align-attr.c
cfe/trunk/test/Sema/alloc-align-attr.c
cfe/trunk/test/SemaCXX/alloc-align-attr.cpp
Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -8176,6 +8176,11 @@
void AddAssumeAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E, Expr *OE,
unsigned SpellingListIndex);
+ /// AddAllocAlignAttr - Adds an alloc_align attribute to a particular
+ /// declaration.
+ void AddAllocAlignAttr(SourceRange AttrRange, Decl *D, Expr *ParamExpr,
+ unsigned SpellingListIndex);
+
/// AddAlignValueAttr - Adds an align_value attribute to a particular
/// declaration.
void AddAlignValueAttr(SourceRange AttrRange, Decl *D, Expr *E,
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -244,6 +244,36 @@
}];
}
+def AllocAlignDocs : Documentation {
+ let Category = DocCatFunction;
+ let Content = [{
+Use ``__attribute__((alloc_align())`` on a function
+declaration to specify that the return value of the function (which must be a
+pointer type) is at least as aligned as the value of the indicated parameter. The
+parameter is given by its index in the list of formal parameters; the first
+parameter has index 1 unless the function is a C++ non-static member function,
+in which case the first parameter has index 2 to account for the implicit ``this``
+parameter.
+
+.. code-block:: c++
+
+ // The returned pointer has the alignment specified by the first parameter.
+ void *a(size_t align) __attribute__((alloc_align(1)));
+
+ // The returned pointer has the alignment specified by the second parameter.
+ void *b(void *v, size_t align) __attribute__((alloc_align(2)));
+
+ // The returned pointer has the alignment specified by the second visible
+ // parameter, however it must be adjusted for the implicit 'this' parameter.
+ void *Foo::b(void *v, size_t align) __attribute__((alloc_align(3)));
+
+Note that this attribute merely informs the compiler that a function always
+returns a sufficiently aligned pointer. It does not cause the compiler to
+emit code to enforce that alignment. The behavior is undefined if the returned
+poitner is not sufficiently aligned.
+ }];
+}
+
def EnableIfDocs : Documentation {
let Category = DocCatFunction;
let Content = [{
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -1225,6 +1225,14 @@
let Documentation = [AssumeAlignedDocs];
}
+def AllocAlign : InheritableAttr {
+ let Spellings = [GCC<"alloc_align">];
+ let Subjects = SubjectList<[HasFunctionProto], WarnDiag,
+ "ExpectedFunctionWithProtoType">;
+ let Args = [IntArgument<"ParamIndex">];
+ let Documentation = [AllocAlignDocs];
+}
+
def NoReturn : InheritableAttr {
let Spellings = [GCC<"noreturn">, Declspec<"noreturn">];
// FIXME: Does GCC allow this on the function instead?
Index: cfe/trunk/test/SemaCXX/alloc-align-attr.cpp
===
--- cfe/trunk/test/SemaCXX/alloc-align-attr.cpp
+++ cfe/trunk/test/SemaCXX/alloc-align-attr.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct param_num {
+ void* Foo(int a) __attribute__((alloc_align(1))); // expected-error {{'alloc_align' attribute is invalid for the implicit this argument}}
+};
+
+
+template
+struct dependent_ret {
+ T* Foo(int a) __attribute__((alloc_align(2)));// no-warning, ends up being int**.
+ T Foo2(int a) __attribute__((alloc_align(2)));// expected-warning {{'alloc_align' attribute only applies to return values that are pointers or references}}
+};
+
+// Following 2 errors associated only with the 'float' versions below.
+template
+struct dependent_param_struct {
+ void* Foo(T param) __attribute__((alloc_align(2))); // expected-error {{'alloc_align' attribute argument may only refer to a function parameter of integer type}}
+};
+
+template
+void* dependent_param_func(T param)
[PATCH] D29599: Clang Changes for alloc_align
erichkeane added a comment. Thanks guys. I THINK I properly removed the svn properties properly, though, I actually didn't know they existed until just now! Repository: rL LLVM https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Can you drop the svn props on the new files when you commit? I don't think we usually set them, and I've seen commits specifically removing the eol-style props before. https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. With that, LGTM. https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 93537.
erichkeane added a comment.
Added tests as requested by John.
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4348,6 +4348,10 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal =
+ CallArgs[AA->getParamIndex() - 1].RV.getScalarVal();
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2465,6 +2465,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -380,6 +390,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,45 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ SourceLocation>::type
+getAttrLoc(const AttrInfo ) {
+ return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList ) {
+ return Attr.getLoc();
+}
+
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ const AttrInfo *>::type
+getAttrName(const AttrInfo ) {
+ return
+}
+const IdentifierInfo *getAttrName(const clang::AttributeList ) {
+ return Attr.getName();
+}
+
/// \brief If Expr is a valid integer constant, get the value of the integer
/// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema , const AttributeList ,
-const Expr *Expr, uint32_t ,
-unsigned Idx = UINT_MAX) {
+template
+static bool checkUInt32Argument(Sema , const AttrInfo& Attr, const Expr *Expr,
+uint32_t , unsigned Idx = UINT_MAX) {
llvm::APSInt I(32);
if (Expr->isTypeDependent() || Expr->isValueDependent() ||
!Expr->isIntegerConstantExpr(I, S.Context)) {
if (Idx != UINT_MAX)
- S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
-<< Attr.getName() << Idx <<
[PATCH] D29599: Clang Changes for alloc_align
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:4363 +} else if (AllocAlignParam && TargetDecl->hasAttr()) + EmitAlignmentAssumption(Ret.getScalarVal(), AllocAlignParam); } rjmccall wrote: > Your old code was fine, you just needed to get the value with > CallArgs[index].second.getScalarVal() instead of IRCallArgs[index]. Please add the test cases I suggested here. You might have to make part of the test target-specific. https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 93491.
erichkeane marked 3 inline comments as done.
erichkeane added a comment.
Thanks for the feedback John!
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4348,6 +4348,10 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal =
+ CallArgs[AA->getParamIndex() - 1].RV.getScalarVal();
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2465,6 +2465,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -380,6 +390,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,45 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ SourceLocation>::type
+getAttrLoc(const AttrInfo ) {
+ return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList ) {
+ return Attr.getLoc();
+}
+
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ const AttrInfo *>::type
+getAttrName(const AttrInfo ) {
+ return
+}
+const IdentifierInfo *getAttrName(const clang::AttributeList ) {
+ return Attr.getName();
+}
+
/// \brief If Expr is a valid integer constant, get the value of the integer
/// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema , const AttributeList ,
-const Expr *Expr, uint32_t ,
-unsigned Idx = UINT_MAX) {
+template
+static bool checkUInt32Argument(Sema , const AttrInfo& Attr, const Expr *Expr,
+uint32_t , unsigned Idx = UINT_MAX) {
llvm::APSInt I(32);
if (Expr->isTypeDependent() || Expr->isValueDependent() ||
!Expr->isIntegerConstantExpr(I, S.Context)) {
if (Idx != UINT_MAX)
- S.Diag(Attr.getLoc(),
[PATCH] D29599: Clang Changes for alloc_align
rjmccall added inline comments. Comment at: include/clang/Basic/AttrDocs.td:252 +declaration to specify that the return value of the function (which must be a +pointer type) is at least as aligned as the value indicated parameter. The +parameter is given by its index in the list of formal parameters; the first "value *of the* indicated parameter" Comment at: include/clang/Basic/AttrDocs.td:254 +parameter is given by its index in the list of formal parameters; the first +parameter hs index 1 unless the function is a C++ non-static member function, +in which case the first parameter has index 2 to account for the implicit ``this`` "has" Comment at: lib/CodeGen/CGCall.cpp:4363 +} else if (AllocAlignParam && TargetDecl->hasAttr()) + EmitAlignmentAssumption(Ret.getScalarVal(), AllocAlignParam); } Your old code was fine, you just needed to get the value with CallArgs[index].second.getScalarVal() instead of IRCallArgs[index]. https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 93421.
erichkeane marked 2 inline comments as done.
erichkeane added a comment.
Fixes based on John's comments. A little bit of extra work was required to get
the correct Value from the attribute, but impact was minimal.
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -3703,6 +3703,15 @@
llvm::FunctionType *IRFuncTy = Callee.getFunctionType();
+ // Support for getting the Value for the parameter identified
+ // by the AllocAlignAttr.
+ const Decl *TargetDecl = Callee.getAbstractInfo().getCalleeDecl();
+ llvm::Value *AllocAlignParam = nullptr;
+ unsigned AllocAlignParamIndex = (std::numeric_limits::max)();
+ if (TargetDecl)
+if (const auto *AA = TargetDecl->getAttr())
+ AllocAlignParamIndex = AA->getParamIndex() - 1;
+
// 1. Set up the arguments.
// If we're using inalloca, insert the allocation after the stack save.
@@ -4009,6 +4018,9 @@
assert(IRArgPos == FirstIRArg + NumIRArgs);
break;
}
+
+if (ArgNo == AllocAlignParamIndex)
+ AllocAlignParam = IRCallArgs[FirstIRArg];
}
llvm::Value *CalleePtr = Callee.getFunctionPointer();
@@ -4337,7 +4349,6 @@
} ();
// Emit the assume_aligned check on the return value.
- const Decl *TargetDecl = Callee.getAbstractInfo().getCalleeDecl();
if (Ret.isScalar() && TargetDecl) {
if (const auto *AA = TargetDecl->getAttr()) {
llvm::Value *OffsetValue = nullptr;
@@ -4348,7 +4359,8 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
-}
+} else if (AllocAlignParam && TargetDecl->hasAttr())
+ EmitAlignmentAssumption(Ret.getScalarVal(), AllocAlignParam);
}
return Ret;
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2465,6 +2465,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -380,6 +390,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,45 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ SourceLocation>::type
+getAttrLoc(const AttrInfo ) {
+ return Attr.getLocation();
+}
+static SourceLocation
[PATCH] D29599: Clang Changes for alloc_align
erichkeane marked 2 inline comments as done.
erichkeane added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter. The
+alignment parameter is one-based. In the case of member functions, the
rjmccall wrote:
> "that the return value of the function ... is at least as aligned as the
> value of the indicated parameter. The parameter is given by its index
> in the list of formal parameters; the first parameter has index 1 unless
> the function is a C++ non-static member function, in which case the
> first parameter has index 2 to account for the implicit ``this`` parameter."
>
> What's the behavior of this attribute if the given attribute is not a power
> of 2?
> Does it silently have no effect, or is it U.B.?
Thanks for the wording, I like this.
It isn't checked for that, so I'll say UB. A little bit of a caveat emptor
here.
Comment at: include/clang/Basic/AttrDocs.td:270
+condition that the code already ensures is true. It does not cause the compiler
+to enforce the provided alignment assumption.
+ }];
rjmccall wrote:
> "Note that this attribute merely informs the compiler that a function always
> returns a sufficiently aligned pointer. It does not cause the compiler to
> emit
> code to enforce that alignment. The behavior is undefined if the returned
> pointer is not sufficiently aligned."
Thanks again!
Comment at: lib/CodeGen/CGCall.cpp:4352
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
rjmccall wrote:
> IRCallArgs is not one-to-one with the list of formal parameters. You need to
> be taking the value out of CallArgList. Three test cases come to mind:
>
> 1. There's an earlier struct argument which expands to no IR arguments.
> 2. There's an earlier struct argument which expands to multiple IR arguments.
> 3. The alignment argument itself expands to multiple IR arguments. For
> example, __int128_t on x86-64.
Awesome, thanks for the info! I'll change that.
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
rjmccall added a comment.
I see that GCC is up to its same parameter-indexing shenanigans again.
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter. The
+alignment parameter is one-based. In the case of member functions, the
"that the return value of the function ... is at least as aligned as the
value of the indicated parameter. The parameter is given by its index
in the list of formal parameters; the first parameter has index 1 unless
the function is a C++ non-static member function, in which case the
first parameter has index 2 to account for the implicit ``this`` parameter."
What's the behavior of this attribute if the given attribute is not a power of
2?
Does it silently have no effect, or is it U.B.?
Comment at: include/clang/Basic/AttrDocs.td:265
+ // The returned pointer has the alignment specified by the second visible
+ // parameter, however it must be adjusted for the implicit 'this' parameter.
+ void *Foo::b(void *v, size_t align) __attribute__((alloc_align(3)));
"The returned pointer has the alignment specified by the second formal
parameter, 'align'. However, for the purposes of the alloc_align attribute,
the parameter index is 3 because of the implicit 'this' parameter."
Comment at: include/clang/Basic/AttrDocs.td:270
+condition that the code already ensures is true. It does not cause the compiler
+to enforce the provided alignment assumption.
+ }];
"Note that this attribute merely informs the compiler that a function always
returns a sufficiently aligned pointer. It does not cause the compiler to emit
code to enforce that alignment. The behavior is undefined if the returned
pointer is not sufficiently aligned."
Comment at: lib/CodeGen/CGCall.cpp:4352
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
IRCallArgs is not one-to-one with the list of formal parameters. You need to
be taking the value out of CallArgList. Three test cases come to mind:
1. There's an earlier struct argument which expands to no IR arguments.
2. There's an earlier struct argument which expands to multiple IR arguments.
3. The alignment argument itself expands to multiple IR arguments. For
example, __int128_t on x86-64.
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 93409.
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4348,6 +4348,9 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2465,6 +2465,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -380,6 +390,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,45 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ SourceLocation>::type
+getAttrLoc(const AttrInfo ) {
+ return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList ) {
+ return Attr.getLoc();
+}
+
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ const AttrInfo *>::type
+getAttrName(const AttrInfo ) {
+ return
+}
+const IdentifierInfo *getAttrName(const clang::AttributeList ) {
+ return Attr.getName();
+}
+
/// \brief If Expr is a valid integer constant, get the value of the integer
/// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema , const AttributeList ,
-const Expr *Expr, uint32_t ,
-unsigned Idx = UINT_MAX) {
+template
+static bool checkUInt32Argument(Sema , const AttrInfo& Attr, const Expr *Expr,
+uint32_t , unsigned Idx = UINT_MAX) {
llvm::APSInt I(32);
if (Expr->isTypeDependent() || Expr->isValueDependent() ||
!Expr->isIntegerConstantExpr(I, S.Context)) {
if (Idx != UINT_MAX)
- S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
-<< Attr.getName() << Idx << AANT_ArgumentIntegerConstant
+ S.Diag(getAttrLoc(Attr),
[PATCH] D29599: Clang Changes for alloc_align
erichkeane marked 9 inline comments as done.
erichkeane added inline comments.
Comment at: lib/Sema/SemaDeclAttr.cpp:1608-1612
+ IndexVal += 1 + isInstanceMethod(FuncDecl);
+
+ if (!checkParamIsIntegerType(*this, FuncDecl, TmpAttr, ParamExpr, IndexVal,
+ /*AttrArgNo=*/0, /*AllowDependentType=*/true))
+return;
aaron.ballman wrote:
> Hmmm, I think this might be a bit more clean as:
> ```
> QualType Ty = getFunctionOrMethodParamType(D, IndexVal);
> if (!Ty->isIntegralType(Context)) {
> Diag(ParamExpr->getLocStart(), diag::err_attribute_integers_only) <<
> TmpAttr <<
> FuncDecl->getParamDecl(IndexVal)->getSourceRange();
> return;
> }
>
> // We cannot use the Idx returned from checkFunctionOrMethodParameterIndex
> // because that has corrected for the implicit this parameter, and is zero-
> // based. The attribute expects what the user wrote explicitly.
> llvm::APSInt Val;
> ParamExpr->EvaluateAsInt(Val, S.Context);
>
> // Use Val.getZExtValue() when you need what the user wrote.
> ```
> This matches more closely with how other attributes handle the situation
> where the Attr object needs what the user wrote (like format_arg). I hadn't
> noticed that checkParamIsIntegerType() turns around and calls
> checkFunctionOrMethodParameterIndex() again, and this would simplify your
> patch a bit.
>
> What do you think?
Looks better, I hadn't realized checkParamIsIntegerType was redoing work either.
I had to make a pair of small changes to this (including omitting DependentType
from the integral type check), but it otherwise works nicely. Thanks!
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
aaron.ballman added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter. The
+alignment parameter is one-indexed. In the case of member functions, the
Use single spacing instead of double spacing.
Comment at: include/clang/Basic/AttrDocs.td:253
+pointer type) has an alignment specified by the indicated parameter. The
+alignment parameter is one-indexed. In the case of member functions, the
+implicit ``this`` parameter is considered as as the first function parameter.
s/one-index/one-based?
member functions -> nonstatic member functions
Comment at: include/clang/Basic/AttrDocs.td:254
+alignment parameter is one-indexed. In the case of member functions, the
+implicit ``this`` parameter is considered as as the first function parameter.
+
is considered as as the first -> is the first
Comment at: include/clang/Basic/AttrDocs.td:258
+
+ // The returned pointer has the alignment specified by the first parameter
+ void *a(size_t align) __attribute__((alloc_align(1)));
Missing full stop.
Comment at: include/clang/Basic/AttrDocs.td:261
+
+ // The returned pointer has the alignment specified by the second parameter
+ void *b(void* v, size_t align) __attribute__((alloc_align(2)));
Same here.
Comment at: include/clang/Basic/AttrDocs.td:262
+ // The returned pointer has the alignment specified by the second parameter
+ void *b(void* v, size_t align) __attribute__((alloc_align(2)));
+
Pointer should bind to `v` (same below).
Comment at: lib/Sema/SemaDeclAttr.cpp:809
+/// an integral type. Will emit appropriate diagnostics if this returns
+/// false.
+///
This can be hoisted up to the previous line.
Comment at: lib/Sema/SemaDeclAttr.cpp:1530
+ const AttributeList ) {
+ Expr *E = Attr.getArgAsExpr(0);
+ S.AddAllocAlignAttr(Attr.getRange(), D, E,
There's no real need for `E`.
Comment at: lib/Sema/SemaDeclAttr.cpp:1532
+ S.AddAllocAlignAttr(Attr.getRange(), D, E,
+ Attr.getAttributeSpellingListIndex());
+}
This formatting looks off (the args should align).
Comment at: lib/Sema/SemaDeclAttr.cpp:1608-1612
+ IndexVal += 1 + isInstanceMethod(FuncDecl);
+
+ if (!checkParamIsIntegerType(*this, FuncDecl, TmpAttr, ParamExpr, IndexVal,
+ /*AttrArgNo=*/0, /*AllowDependentType=*/true))
+return;
Hmmm, I think this might be a bit more clean as:
```
QualType Ty = getFunctionOrMethodParamType(D, IndexVal);
if (!Ty->isIntegralType(Context)) {
Diag(ParamExpr->getLocStart(), diag::err_attribute_integers_only) << TmpAttr
<<
FuncDecl->getParamDecl(IndexVal)->getSourceRange();
return;
}
// We cannot use the Idx returned from checkFunctionOrMethodParameterIndex
// because that has corrected for the implicit this parameter, and is zero-
// based. The attribute expects what the user wrote explicitly.
llvm::APSInt Val;
ParamExpr->EvaluateAsInt(Val, S.Context);
// Use Val.getZExtValue() when you need what the user wrote.
```
This matches more closely with how other attributes handle the situation where
the Attr object needs what the user wrote (like format_arg). I hadn't noticed
that checkParamIsIntegerType() turns around and calls
checkFunctionOrMethodParameterIndex() again, and this would simplify your patch
a bit.
What do you think?
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 93404.
erichkeane added a comment.
Made the changes as requested. checkFunctionOrMethodParameterIndex corrects
for 1->0 index and implicit this, which requires undoing, otherwise templates
create a big hassle. Additionally, please note the AttrDocs changes, I think
I've got it acceptable but would love a second english-smith.
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4348,6 +4348,9 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2465,6 +2465,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -380,6 +390,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,45 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ SourceLocation>::type
+getAttrLoc(const AttrInfo ) {
+ return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList ) {
+ return Attr.getLoc();
+}
+
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ const AttrInfo *>::type
+getAttrName(const AttrInfo ) {
+ return
+}
+const IdentifierInfo *getAttrName(const clang::AttributeList ) {
+ return Attr.getName();
+}
+
/// \brief If Expr is a valid integer constant, get the value of the integer
/// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema , const AttributeList ,
-const Expr *Expr, uint32_t ,
-unsigned Idx = UINT_MAX) {
+template
+static bool checkUInt32Argument(Sema , const AttrInfo& Attr, const Expr *Expr,
+uint32_t , unsigned Idx = UINT_MAX) {
llvm::APSInt I(32);
if
[PATCH] D29599: Clang Changes for alloc_align
erichkeane marked 2 inline comments as done. erichkeane added inline comments. Comment at: include/clang/Basic/AttrDocs.td:252 +declaration to specify that the return value of the function (which must be a +pointer type) has an alignment specified by the indicated parameter, starting +with 1. aaron.ballman wrote: > aaron.ballman wrote: > > I would split the "starting with 1" off into its own (full) sentence for > > clarity purposes. Also, please spell out one instead of 1. > > > > You may also want to clarify how member functions do/do not impact this > > index. > This does not appear to have been handled? Ah, I missed that you wanted it in its own sentence, not just its own line. I'll expand on this text in the next patch. https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
aaron.ballman added inline comments.
Comment at: include/clang/Basic/Attr.td:1230
+ let Spellings = [GCC<"alloc_align">];
+ let Subjects = SubjectList<[ Function]>;
+ let Args = [IntArgument<"ParamIndex">];
There's a spurious space between [ and Function.
If we want to behave like GCC, then your subject list is fine. If we want to
tighten up what we allow rather than silently accept the attribute and do
nothing with it, this should probably be HasFunctionProto because the attribute
doesn't seem to make sense on an unprototyped function.
Comment at: include/clang/Basic/Attr.td:1224
+def AllocAlign : InheritableAttr {
+ let Spellings = [ GCC<"alloc_align"> ];
+ let Subjects = SubjectList<[Function]>;
erichkeane wrote:
> aaron.ballman wrote:
> > Extra spaces in the declaration that do not match the style of the rest of
> > the file (same happens below).
> Strangely, this is ClangFormat at work :/
Ah, I guess clang-format doesn't quite understand .td files.
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter, starting
+with 1.
aaron.ballman wrote:
> I would split the "starting with 1" off into its own (full) sentence for
> clarity purposes. Also, please spell out one instead of 1.
>
> You may also want to clarify how member functions do/do not impact this index.
This does not appear to have been handled?
Comment at: lib/CodeGen/CGCall.cpp:4374
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
erichkeane wrote:
> aaron.ballman wrote:
> > Instead of hoping all of the callers of `getParamIndex()` will remember
> > that this is a weird one-based thing, why not give the semantic attribute
> > the correct index when attaching the attribute to the declaration?
> I played with this for a while, and the difficulty is that AddAllocAlignAttr
> requires the 1-based index, since it needs to error based on that number.
> Additionally, decrementing the value BEFORE that function becomes difficult,
> since it is an Expr object at that time (which would get messy in the
> template case).
>
> I cannot alter it in the AddAllocAlignAttr function, since that function
> actually gets called TWICE in the template instantiation case, so I'd likely
> need some sort of flag that told whether to treat it as decremented or not.
> In general, this gets really ugly really quickly.
>
> Basically, I don't see a good place to decrement the value anywhere without
> causing a much more significant change.
Thank you for the explanation, that makes sense to me.
Comment at: lib/Sema/SemaDeclAttr.cpp:1599
+ int IndexVal;
+ if (!checkPositiveIntArgument(*this, TmpAttr, ParamExpr, IndexVal,
+/*Index=*/1))
erichkeane wrote:
> aaron.ballman wrote:
> > It seems strange to me that you check that it's a positive integer argument
> > before checking the param is an integer type.
> >
> > Why not use `checkFunctionOrMethodParameterIndex()`?
> I'm unaware of checkFunctionOrMethodParameterIndex, there are a ton of odd
> free fucntions around here, I just copied from some of the surrounding
> functions.
>
> That said, these two poorly named functions are actually checking 2 different
> pieces of data. So in the case of:
> void func(int foo) __attribute__((alloc_align(1));
>
> The "checkPositiveIntArgument" checks to ensure that '1' is a positive
> integer. "checkParamIsIntegerType" checks that "foo" (the corresponding
> parameter) is an integer, and that '1' is in range.
Ah, I see (and yes, that function name is rather ambiguous). I think you should
be using `checkFunctionOrMethodParameterIndex()` in place of
`checkPositiveIntArgument()` and leave in the `checkParamIsIntegerType()`.
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 93158.
erichkeane marked 2 inline comments as done.
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4348,6 +4348,9 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2465,6 +2465,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -380,6 +390,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,45 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ SourceLocation>::type
+getAttrLoc(const AttrInfo ) {
+ return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList ) {
+ return Attr.getLoc();
+}
+
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList.
+template
+static typename std::enable_if::value,
+ const AttrInfo *>::type
+getAttrName(const AttrInfo ) {
+ return
+}
+const IdentifierInfo *getAttrName(const clang::AttributeList ) {
+ return Attr.getName();
+}
+
/// \brief If Expr is a valid integer constant, get the value of the integer
/// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema , const AttributeList ,
-const Expr *Expr, uint32_t ,
-unsigned Idx = UINT_MAX) {
+template
+static bool checkUInt32Argument(Sema , const AttrInfo& Attr, const Expr *Expr,
+uint32_t , unsigned Idx = UINT_MAX) {
llvm::APSInt I(32);
if (Expr->isTypeDependent() || Expr->isValueDependent() ||
!Expr->isIntegerConstantExpr(I, S.Context)) {
if (Idx != UINT_MAX)
- S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
-<< Attr.getName() << Idx << AANT_ArgumentIntegerConstant
+
[PATCH] D29599: Clang Changes for alloc_align
erichkeane marked 15 inline comments as done.
erichkeane added a comment.
Comments on 2 cases, otherwise a Patch incoming that fixes the rest of Aaron's
comments.
Comment at: include/clang/Basic/Attr.td:1224
+def AllocAlign : InheritableAttr {
+ let Spellings = [ GCC<"alloc_align"> ];
+ let Subjects = SubjectList<[Function]>;
aaron.ballman wrote:
> Extra spaces in the declaration that do not match the style of the rest of
> the file (same happens below).
Strangely, this is ClangFormat at work :/
Comment at: include/clang/Basic/Attr.td:1225
+ let Spellings = [ GCC<"alloc_align"> ];
+ let Subjects = SubjectList<[Function]>;
+ let Args = [ IntArgument<"ParamIndex"> ];
aaron.ballman wrote:
> Is this subject line correct, or should it be using `HasFunctionProto`
> instead? How does GCC handle something like `void *blah()
> __attribute__((alloc_align(1)));` in C code?
I'm not sure what HasFunctionProto means in this case.
Either way, GCC does basically ZERO SEMA here, so GCC would allow that to
happen, then simply ignore the attribute. We're enforcing that in the SEMA
changes below.
Comment at: lib/CodeGen/CGCall.cpp:4374
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
aaron.ballman wrote:
> Instead of hoping all of the callers of `getParamIndex()` will remember that
> this is a weird one-based thing, why not give the semantic attribute the
> correct index when attaching the attribute to the declaration?
I played with this for a while, and the difficulty is that AddAllocAlignAttr
requires the 1-based index, since it needs to error based on that number.
Additionally, decrementing the value BEFORE that function becomes difficult,
since it is an Expr object at that time (which would get messy in the template
case).
I cannot alter it in the AddAllocAlignAttr function, since that function
actually gets called TWICE in the template instantiation case, so I'd likely
need some sort of flag that told whether to treat it as decremented or not. In
general, this gets really ugly really quickly.
Basically, I don't see a good place to decrement the value anywhere without
causing a much more significant change.
Comment at: lib/Sema/SemaDeclAttr.cpp:806
+/// \brief Checks to be sure that the given parameter number is inbounds, and
is
+/// an some integral type. Will emit appropriate diagnostics if this returns
+/// false.
aaron.ballman wrote:
> Remove the "some".
This is what I get for just C'ing the existing comment :)
Comment at: lib/Sema/SemaDeclAttr.cpp:1599
+ int IndexVal;
+ if (!checkPositiveIntArgument(*this, TmpAttr, ParamExpr, IndexVal,
+/*Index=*/1))
aaron.ballman wrote:
> It seems strange to me that you check that it's a positive integer argument
> before checking the param is an integer type.
>
> Why not use `checkFunctionOrMethodParameterIndex()`?
I'm unaware of checkFunctionOrMethodParameterIndex, there are a ton of odd free
fucntions around here, I just copied from some of the surrounding functions.
That said, these two poorly named functions are actually checking 2 different
pieces of data. So in the case of:
void func(int foo) __attribute__((alloc_align(1));
The "checkPositiveIntArgument" checks to ensure that '1' is a positive integer.
"checkParamIsIntegerType" checks that "foo" (the corresponding parameter) is
an integer, and that '1' is in range.
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
aaron.ballman added inline comments.
Comment at: include/clang/Basic/Attr.td:1224
+def AllocAlign : InheritableAttr {
+ let Spellings = [ GCC<"alloc_align"> ];
+ let Subjects = SubjectList<[Function]>;
Extra spaces in the declaration that do not match the style of the rest of the
file (same happens below).
Comment at: include/clang/Basic/Attr.td:1225
+ let Spellings = [ GCC<"alloc_align"> ];
+ let Subjects = SubjectList<[Function]>;
+ let Args = [ IntArgument<"ParamIndex"> ];
Is this subject line correct, or should it be using `HasFunctionProto` instead?
How does GCC handle something like `void *blah()
__attribute__((alloc_align(1)));` in C code?
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter, starting
+with 1.
I would split the "starting with 1" off into its own (full) sentence for
clarity purposes. Also, please spell out one instead of 1.
You may also want to clarify how member functions do/do not impact this index.
Comment at: include/clang/Basic/AttrDocs.td:256
+.. code-block:: c++
+ // The returned pointer has the alignment specified by the first parameter
+ void *a(size_t align) __attribute__((alloc_align(1)));
I think you need a newline above this code block to not trigger sphinx
diagnostics.
Comment at: lib/CodeGen/CGCall.cpp:4374
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
Instead of hoping all of the callers of `getParamIndex()` will remember that
this is a weird one-based thing, why not give the semantic attribute the
correct index when attaching the attribute to the declaration?
Comment at: lib/CodeGen/CGCall.cpp:4377
}
+
}
Spurious newline should be removed.
Comment at: lib/Sema/SemaDeclAttr.cpp:222
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList
+template static typename
Missing a full stop at the end of the comment.
Comment at: lib/Sema/SemaDeclAttr.cpp:223
+/// AND the AttributeList
+template static typename
+std::enable_if::value,
SourceLocation>::type
s/class/typename (same below).
Also, add some newlines between the function definitions and run the patch
through clang-format (pointers and references are bound to the wrong thing and
the formatting seems a bit off).
Comment at: lib/Sema/SemaDeclAttr.cpp:232
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList
+template static typename
Missing a full stop at the end of the comment.
Comment at: lib/Sema/SemaDeclAttr.cpp:775
}
-
+//
/// \brief Checks to be sure that the given parameter number is inbounds, and
is
Should be removed.
Comment at: lib/Sema/SemaDeclAttr.cpp:793
const ParmVarDecl *Param = FD->getParamDecl(Idx);
+ if (AllowDependentType && Param->getType()->isDependentType()) {
+return true;
Can elide the braces.
Comment at: lib/Sema/SemaDeclAttr.cpp:805
+/// \brief Checks to be sure that the given parameter number is inbounds, and
is
+/// an some integral type. Will emit appropriate diagnostics if this returns
s/inbounds/in bounds
Comment at: lib/Sema/SemaDeclAttr.cpp:806
+/// \brief Checks to be sure that the given parameter number is inbounds, and
is
+/// an some integral type. Will emit appropriate diagnostics if this returns
+/// false.
Remove the "some".
Comment at: lib/Sema/SemaDeclAttr.cpp:1599
+ int IndexVal;
+ if (!checkPositiveIntArgument(*this, TmpAttr, ParamExpr, IndexVal,
+/*Index=*/1))
It seems strange to me that you check that it's a positive integer argument
before checking the param is an integer type.
Why not use `checkFunctionOrMethodParameterIndex()`?
Comment at: test/Sema/alloc-align-attr.c:5
+void test_void_alloc_align(void) __attribute__((alloc_align(1))); //
expected-warning {{'alloc_align' attribute only applies to return values that
are pointers}}
+int test_int_alloc_align(void) __attribute__((alloc_align(1))); //
expected-warning {{'alloc_align' attribute only applies to return values that
are pointers}}
+void *test_ptr_alloc_align(int a) __attribute__((alloc_align(1))); //
no-warning
This test doesn't really add value.
Comment at:
[PATCH] D29599: Clang Changes for alloc_align
erichkeane added a comment. FWIW, the LLVM changes for this have been committed. https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 92814.
erichkeane added a comment.
Update test based on the corresponding LLVM change.
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4370,7 +4370,11 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
+
}
return Ret;
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2465,6 +2465,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -352,6 +362,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,42 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList
+template static typename
+std::enable_if::value, SourceLocation>::type
+getAttrLoc(const AttrInfo& Attr) {
+ return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList& Attr) {
+ return Attr.getLoc();
+}
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList
+template static typename
+std::enable_if::value, const AttrInfo*>::type
+getAttrName(const AttrInfo& Attr) {
+ return
+}
+const IdentifierInfo* getAttrName(const clang::AttributeList ) {
+ return Attr.getName();
+}
+
/// \brief If Expr is a valid integer constant, get the value of the integer
/// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema , const AttributeList ,
-const Expr *Expr, uint32_t ,
-unsigned Idx = UINT_MAX) {
+template
+static bool checkUInt32Argument(Sema , const AttrInfo& Attr, const Expr *Expr,
+uint32_t , unsigned Idx = UINT_MAX) {
llvm::APSInt I(32);
if (Expr->isTypeDependent() || Expr->isValueDependent() ||
!Expr->isIntegerConstantExpr(I, S.Context)) {
if (Idx != UINT_MAX)
- S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
-<< Attr.getName() << Idx << AANT_ArgumentIntegerConstant
+
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 92709.
erichkeane marked an inline comment as done.
erichkeane added a comment.
Thanks for the review! Tests updated to clarify the cast htat is happening.
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4370,7 +4370,11 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
+
}
return Ret;
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2465,6 +2465,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -352,6 +362,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,42 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList
+template static typename
+std::enable_if::value, SourceLocation>::type
+getAttrLoc(const AttrInfo& Attr) {
+ return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList& Attr) {
+ return Attr.getLoc();
+}
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList
+template static typename
+std::enable_if::value, const AttrInfo*>::type
+getAttrName(const AttrInfo& Attr) {
+ return
+}
+const IdentifierInfo* getAttrName(const clang::AttributeList ) {
+ return Attr.getName();
+}
+
/// \brief If Expr is a valid integer constant, get the value of the integer
/// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema , const AttributeList ,
-const Expr *Expr, uint32_t ,
-unsigned Idx = UINT_MAX) {
+template
+static bool checkUInt32Argument(Sema , const AttrInfo& Attr, const Expr *Expr,
+uint32_t , unsigned Idx = UINT_MAX) {
llvm::APSInt I(32);
if (Expr->isTypeDependent() || Expr->isValueDependent() ||
!Expr->isIntegerConstantExpr(I, S.Context)) {
if (Idx != UINT_MAX)
- S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
-<<
[PATCH] D29599: Clang Changes for alloc_align
myatsina added inline comments.
Comment at: test/CodeGen/alloc-align-attr.c:19
+}
+// Condition where test2 param needs casting.
+__INT32_TYPE__ test2(__SIZE_TYPE__ a) {
Where exactly do we see this test2 param casting?
I think you have a missing check before the m1 call
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane added a comment. Ping! https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane updated this revision to Diff 88602.
erichkeane added a comment.
I was able to get the templated versions working in response to the discussion
with Akira. Note the added test file which shows off all of the combos I could
think of.
It required a little bit of surgery inside the SemaDeclAttr.cpp, since the
SemaTemplateInstatiateDecl.cpp no longer has "AttributeList" info anymore, so
getting the error messages in the existing functions required a little
template-writing of my own!
I decided to explicitly forbid the following case, because I cannot see a valid
usecase for this, or for making 'Which' below a dependent value.
template
void* foo(int a, int b, int c, int d) __attribute__((alloc_align(Which)));
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
test/SemaCXX/alloc-align-attr.cpp
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4225,7 +4225,11 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
+
}
return Ret;
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2412,6 +2412,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -168,6 +168,16 @@
Aligned->getSpellingListIndex());
}
+static void instantiateDependentAllocAlignAttr(
+Sema , const MultiLevelTemplateArgumentList ,
+const AllocAlignAttr *Align, Decl *New) {
+ Expr *Param = IntegerLiteral::Create(
+ S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+ S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+ Align->getSpellingListIndex());
+}
+
static Expr *instantiateDependentFunctionAttrCondition(
Sema , const MultiLevelTemplateArgumentList ,
const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
@@ -352,6 +362,12 @@
continue;
}
+if (const auto *AllocAlign = dyn_cast(TmplAttr)) {
+ instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+ continue;
+}
+
+
if (const auto *EnableIf = dyn_cast(TmplAttr)) {
instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
cast(New));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -218,21 +218,42 @@
std::greater());
}
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList
+template static typename
+std::enable_if::value, SourceLocation>::type
+getAttrLoc(const AttrInfo& Attr) {
+ return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList& Attr) {
+ return Attr.getLoc();
+}
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList
+template static typename
+std::enable_if::value, const AttrInfo*>::type
+getAttrName(const AttrInfo& Attr) {
+ return
+}
+const IdentifierInfo* getAttrName(const clang::AttributeList ) {
+ return Attr.getName();
+}
+
/// \brief If Expr is a valid integer constant, get the value of the integer
/// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema , const
[PATCH] D29599: Clang Changes for alloc_align
erichkeane added a comment. Ah, I see! Sorry for missing that. I don't see a reason why we cannot support that, but I wasn't really considering it. In general, this attribute is a compiler hint for some C standard library stuff in glibc. I've been playing with it a few hours now, and it seems that GCC ONLY complains about the attribute parameter being in range (same error for the type of the parameter as well!), so we DO a bit extra SEMA here, though I think that is correct. I'll dig into this a bit more and see if I can get the template return type working correctly. https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
ahatanak added a comment.
My question probably wasn't clear, but I wasn't sure how template functions in
general (not just member functions) should be handled.
I see a warning when the following function is compiled:
template
T __attribute__((alloc_align(1))) foo0(int a) {
typedef typename std::remove_pointer::type T2;
return new T2();
}
void foo1() {
foo0(1);
}
clang complains that the attribute only applies to functions returning pointers
or references, but foo0 does return a pointer. Is that intended?
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane added a comment.
In https://reviews.llvm.org/D29599#674772, @ahatanak wrote:
> Can this attribute be used on c++ template methods? Is the following code
> valid?
>
> template
> struct S {
> T foo(int a) __attribute__((alloc_align(1)));
> };
>
Yes it can, however that example will NOT compile. First, on member functions
you cannot use alloc_align to refer to the 'this' function. Second, the return
value must be a pointer, so if remove_pointer == T, this will fail.
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
ahatanak added a comment.
Can this attribute be used on c++ template methods? Is the following code valid?
template
struct S {
T foo(int a) __attribute__((alloc_align(1)));
};
https://reviews.llvm.org/D29599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane added a comment. Requires this LLVM commit: https://reviews.llvm.org/D29598 https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29599: Clang Changes for alloc_align
erichkeane created this revision.
GCC has the alloc_align attribute, which is similar to assume_aligned, except
the attribute's parameter is the index of the integer parameter that needs
aligning to.
https://reviews.llvm.org/D29599
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaDeclAttr.cpp
test/CodeGen/alloc-align-attr.c
test/Sema/alloc-align-attr.c
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -4225,7 +4225,11 @@
llvm::ConstantInt *AlignmentCI = cast(Alignment);
EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
OffsetValue);
+} else if (const auto *AA = TargetDecl->getAttr()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
}
+
}
return Ret;
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2412,6 +2412,12 @@
PeepholeProtection protectFromPeepholes(RValue rvalue);
void unprotectFromPeepholes(PeepholeProtection protection);
+ void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+ llvm::Value *OffsetValue = nullptr) {
+Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+ OffsetValue);
+ }
+
//======//
// Statement Emission
//======//
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -1484,6 +1484,13 @@
Attr.getAttributeSpellingListIndex());
}
+static void handleAllocAlignAttr(Sema , Decl *D,
+ const AttributeList ) {
+ Expr *E = Attr.getArgAsExpr(0);
+ S.AddAllocAlignAttr(Attr.getRange(), D, E, Attr,
+ Attr.getAttributeSpellingListIndex());
+}
+
void Sema::AddAssumeAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E,
Expr *OE, unsigned SpellingListIndex) {
QualType ResultType = getFunctionOrMethodResultType(D);
@@ -1535,6 +1542,34 @@
AssumeAlignedAttr(AttrRange, Context, E, OE, SpellingListIndex));
}
+void Sema::AddAllocAlignAttr(SourceRange AttrRange, Decl *D, Expr *ParamExpr,
+ const AttributeList ,
+ unsigned SpellingListIndex) {
+ QualType ResultType = getFunctionOrMethodResultType(D);
+ SourceRange SR = getFunctionOrMethodResultSourceRange(D);
+
+ AllocAlignAttr TmpAttr(AttrRange, Context, 0, SpellingListIndex);
+ SourceLocation AttrLoc = AttrRange.getBegin();
+
+ if (!isValidPointerAttrType(ResultType, /* RefOkay */ true)) {
+Diag(AttrLoc, diag::warn_attribute_return_pointers_refs_only)
+<< << AttrRange << SR;
+return;
+ }
+
+ int IndexVal;
+ if (!checkPositiveIntArgument(*this, Attr, ParamExpr, IndexVal, /*Index=*/1))
+return;
+
+ const auto *FuncDecl = cast(D);
+ if (!checkParamIsIntegerType(*this, FuncDecl, Attr, IndexVal,
+ /*AttrArgNo=*/0))
+return;
+
+ D->addAttr(::new (Context) AllocAlignAttr(AttrRange, Context, IndexVal,
+SpellingListIndex));
+}
+
/// Normalize the attribute, __foo__ becomes foo.
/// Returns true if normalization was applied.
static bool normalizeName(StringRef ) {
@@ -5837,6 +5872,9 @@
case AttributeList::AT_AssumeAligned:
handleAssumeAlignedAttr(S, D, Attr);
break;
+ case AttributeList::AT_AllocAlign:
+handleAllocAlignAttr(S, D, Attr);
+break;
case AttributeList::AT_Overloadable:
handleSimpleAttribute(S, D, Attr);
break;
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -8125,6 +8125,11 @@
void AddAssumeAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E, Expr *OE,
unsigned SpellingListIndex);
+ /// AddAllocAlignAttr - Adds an alloc_align attribute to a particular
+ /// declaration.
+ void AddAllocAlignAttr(SourceRange AttrRange, Decl *D, Expr *ParamExpr,
+ const AttributeList , unsigned SpellingListIndex);
+
/// AddAlignValueAttr - Adds an align_value attribute to a particular
/// declaration.
void AddAlignValueAttr(SourceRange AttrRange, Decl *D, Expr *E,
Index: include/clang/Basic/AttrDocs.td
