[cfe-dev] aligned 'new'

[Anton Yartsev]

Thanks a lot for the review!
> +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.
done

> +
> +// 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)
>
>
> This is a non-static, non-inline function definition in a header, which can't be right.
oops.. fixed

> Besides, shouldn't the default implementation for this function be part of the C++ support library, where the default operator new is defined?
Putting this functions to clang header makes the feature independent of 
library used

> +    // 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?
fixed

> +      // 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.
done

> @@ -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 the call to __aligned_... fails we end up with an error ('no matching 
function for call to ...'). This behavior seem correct to me as 
specifying -fstrict-aligned option we expect our data to be aligned 
properly.

> -    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.
I got a crash here when gave clang bad sources to compile and clang 
treated __aligned... function as variable declaration while trying to 
recover from errors.

> 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.
added the test.

That is how the feature is designed in ppu-lv2-gcc:
giving -fstrict-align option to ppu-lv2-gcc does the following:
1) It cause ppu-lv2-gcc to define the built-in macro "__STRICT_ALIGNED".
2) The PS3 header "new" has some new declarations for new and delete 
operators (standard, nothrow and placement, both simple and array 
versions) that take an additional size_t parameter for alignment. These 
are conditionalized on the __STRICT_ALIGNED flag.
3) If this option is set, when ppu-lv2-gcc sees a new or delete of a 
type which either has an inherent alignment, or has an explicit 
alignment specified by __attribute__((__aligned__(x))), it will 
transform the standard new or delete call to a call to the aligned 
version of new or delete.

Please review and comment.
Tell if I can commit anything from the patch.

-- 
Anton

-------------- next part --------------
A non-text attachment was scrubbed...
Name: aligned_new_v1.patch
Type: text/x-diff
Size: 16015 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110902/6183bee2/attachment.patch>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: aligned_new.cpp
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110902/6183bee2/attachment.ksh>