[cfe-dev] aligned 'new'
Douglas Gregor
dgregor at apple.com
Fri Aug 26 08:38:26 PDT 2011
On Aug 23, 2011, at 9:53 AM, Anton Yartsev wrote:
> On 23.08.2011 20:12, Chris Lattner wrote:
>> On Aug 23, 2011, at 2:15 AM, Anton Yartsev wrote:
>>
>>>>>>> Attached is the the patch that substitutes calls to default new operators with calls to functions - aligned analogs of default new operators. This functions may live in the clang's mm_malloc.h header where aligned malloc lives. The patch is partial, it does not affect deletes and do not contain substituting functions yet. Please review and direct!
>>>>>>>
>>>>>> Ping!
>>>>>>
>>>>> Hi,
>>>>>
>>>>> here is the updated patch that substitute both news and deletes, please review!
>>>>>
>>>> Final patch with aligned functions added, please review!
>>>>
>>> Ping!
>> Hi Anton,
>>
>> Can you include the most recent patch?
>>
>> -Chris
> Attached
I have a few specific comments about the patch, and a meta-comment below.
+bool FunctionDecl::isAlignedSubstitutionOfNewDelete(bool CheckNew,
+ bool CheckNewArr,
+ bool CheckDelete,
+ bool CheckDeleteArr) const {
+
+ if (!getASTContext().getLangOptions().StrictAligned)
+ return false;
+
+ std::string Name = getDeclName().getAsString();
+
+ return ((CheckNew && !Name.compare("__aligned_new")) ||
+ (CheckNewArr && !Name.compare("__aligned_new_arr")) ||
+ (CheckDelete && !Name.compare("__aligned_delete")) ||
+ (CheckDeleteArr && !Name.compare("__aligned_delete_arr")));
+}
DeclarationName::getAsString() is not an efficient operation. Please use getAsIdentifierInfo() and compare against that. If this routine will be called often, it would be far better if this comparison where a pointer comparison (against a known IdentifierInfo*) rather than a string comparison.
Index: lib/Headers/mm_malloc.h
===================================================================
--- lib/Headers/mm_malloc.h (revision 138334)
+++ lib/Headers/mm_malloc.h (working copy)
@@ -25,6 +25,7 @@
#define __MM_MALLOC_H
#include <stdlib.h>
+#include <new>
The inclusion of <new> needs to at least be behind and #ifdef __cplusplus, so it doesn't break C code.
+
+// Alligned analogs of standard library operator new functions declared
+// in the <new> header
+void *__aligned_new(std::size_t size, std::size_t align) throw (std::bad_alloc)
+{
+ if (size == 0)
+ size = 1;
+
+ void* p = 0;
+
+ while ((p = _mm_malloc(size, align)) == 0) {
+ // If _mm_malloc fails and there is a new_handler,
+ // call it to try free up memory.
+ std::new_handler nh = std::set_new_handler(0);
+ if (nh) {
+ std::set_new_handler(nh);
+ nh();
+ }
+ else
+ throw std::bad_alloc();
+ }
+ return p;
+}
This is a non-static, non-inline function definition in a header, which can't be right. Besides, shouldn't the default implementation for this function be part of the C++ support library, where the default operator new is defined?
+ // Call to a new operator was replaced with the call to __aligned_new or
+ // __aligned_new_arr function.
+ // Add alignment argument in front of placement arguments
+ if (getLangOptions().StrictAligned &&
+ OperatorNew->isAlignedSubstitutionOfNewDelete(
+ /*new*/true, /*new[]*/true, /*delete*/false, /*delete[]*/false)) {
+
+ unsigned Alignment =
+ Context.getTypeAlignInChars(AllocType).getQuantity();
+
+ Expr* AlignmentArg =
+ IntegerLiteral::Create(Context, llvm::APInt(
+ Context.Target.getIntWidth(), Alignment),
+ Context.IntTy, SourceLocation());
Since the alignment parameters have type std::size_t, shouldn't the IntegerLiteral be created with the size type?
+ // nothrow new
+ if (const ReferenceType *Ref = secontParamType->getAs<ReferenceType>())
+ if (const RecordType *Rec = Ref->getPointeeType()->getAs<RecordType>())
+ if (Rec->getDecl()->getDeclName().getAsString() == "nothrow_t")
+ if (NamespaceDecl *Namespace =
+ dyn_cast<NamespaceDecl>(Rec->getDecl()->getParent()))
+ if (Namespace->getDeclName().getAsString() == "std")
+ return true;
String comparisons on type names aren't something we do in Sema except in extreme circumstances. Please find a better way to identify nothrow.
@@ -1282,10 +1354,54 @@
// Didn't find a member overload. Look for a global one.
DeclareGlobalNewDelete();
DeclContext *TUDecl = Context.getTranslationUnitDecl();
+
if (FindAllocationOverload(StartLoc, Range, NewName, &AllocArgs[0],
AllocArgs.size(), TUDecl, /*AllowMissing=*/false,
OperatorNew))
return true;
+
+ // replace calls to allocation functions defined in the <new> header
+ // with calls to its aligned analogs
+ if (getLangOptions().StrictAligned &&
+ isDefaultLibraryAllocationFunction(OperatorNew)) {
+
+ OperatorNew = 0;
+
+ DeclarationName AlignedNewName(
+ &Context.Idents.get(IsArray ? "__aligned_new_arr" : "__aligned_new"));
+
+ DeclarationName AlignedDeleteName(
+ &Context.Idents.get(IsArray ? "__aligned_delete_arr"
+ : "__aligned_delete"));
+
+ // construct additional parameter - alignment
+ IntegerLiteral AlignmentArg(Context, llvm::APInt::getNullValue(
+ Context.Target.getPointerWidth(0)),
+ Context.getSizeType(),
+ SourceLocation());
+
+ llvm::SmallVector<Expr*, 8> ArgsPlusAlign(AllocArgs);
+
+ // insert alignment parameter just after the Size parameter
+ ArgsPlusAlign.insert(ArgsPlusAlign.begin()+1, &AlignmentArg);
+
+ if (FindAllocationOverload(StartLoc, Range, AlignedNewName,
+ &ArgsPlusAlign[0], ArgsPlusAlign.size(), TUDecl,
+ /*AllowMissing=*/false, OperatorNew))
+ return true;
There's no fallback here, which seems to imply that if the call to __aligned_new/__aligned_new_arr/etc. fails, we won't end up trying to the standard-mandated operator new/operator new[]/etc. Is that really the intended behavior? It seems like fairly significant breakage to me.
+ if (getLangOptions().Exceptions) {
+ llvm::SmallVector<Expr*, 8> DeallocArgs(AllocArgs);
+ CXXNullPtrLiteralExpr VoidPtr(Context.VoidPtrTy, SourceLocation());
+ DeallocArgs[0] = &VoidPtr;
+ FindAllocationOverload(StartLoc, Range, AlignedDeleteName,
+ &DeallocArgs[0], DeallocArgs.size(), TUDecl,
+ /*AllowMissing*/true, OperatorDelete,
+ /*Diagnose*/false);
+ }
+
+ return false;
+ }
}
@@ -1460,9 +1576,9 @@
continue;
}
- FunctionDecl *Fn = cast<FunctionDecl>(D);
- AddOverloadCandidate(Fn, Alloc.getPair(), Args, NumArgs, Candidates,
- /*SuppressUserConversions=*/false);
+ if (FunctionDecl *Fn = dyn_cast<FunctionDecl>(D))
+ AddOverloadCandidate(Fn, Alloc.getPair(), Args, NumArgs, Candidates,
+ /*SuppressUserConversions=*/false);
}
Why is this change necessary? Is seems like an invariant has been broken, if this change did anything.
@@ -1951,6 +2075,13 @@
DeclareGlobalNewDelete();
DeclContext *TUDecl = Context.getTranslationUnitDecl();
Expr *Arg = Ex.get();
+
+ if (getLangOptions().StrictAligned)
+ // Substitute call to operator delete with call to its aligned analog
+ DeleteName = DeclarationName(
+ &Context.Idents.get(ArrayForm ? "__aligned_delete_arr"
+ : "__aligned_delete"));
+
if (FindAllocationOverload(StartLoc, SourceRange(), DeleteName,
&Arg, 1, TUDecl, /*AllowMissing=*/false,
OperatorDelete))
Again, we don't seem to have a fallback to the standard-mandated behavior, which seems problematic to me.
Now, the meta-level point: this is a significant extension to Clang, so it needs proper documentation, a specification, a test suite, etc., as described in
http://clang.llvm.org/get_involved.html
You mentioned that this is a pre-existing feature for ppu-lv2-gcc from the Sony Cell SDK, which likely means that the design of the feature is already fairly constrained. That said, we need to understand the design of the feature before we can evaluate the implementation, and I still don't have a good sense of how the feature is supposed to work. More documentation and examples would help a lot.
- Doug
More information about the cfe-dev
mailing list