[cfe-dev] aligned 'new'
Anton Yartsev
anton.yartsev at gmail.com
Tue Sep 20 11:37:45 PDT 2011
Here is an updated patch with lots of changes. Tried to make it of
higher quality. Please review.
aligned_new_test.cpp - new test from clang\test\CodeGenCXX\
aligned_new.h - header with new declarations from clang\lib\Headers\
HelperLibs.zip - files for creating library. Placed it to clang\lib\
>> Putting this functions to clang header makes the feature independent of library used
> Yes, but it has several unfortunate side effects:
>
> (1) We'll end up with copies of these functions in every .o file that uses new or delete, which is likely to cause some serious code bloat
> (2) Unlike with the default new/new[]/delete/delete[] operators provided by the C++ implementation, these 'aligned' versions are not replaceable by the user because their definitions are internal to each .o file
>
> There are other ways to achieve library independence. For example, the definitions could be in a separate library that we link against when -fstrict-aligned is provided, and the declarations themselves could be synthesized by the compiler when needed (e.g., as a library built-in).
Made standard non-placement declarations synthesized by the compiler as
it is made for standard non-placement new/new[]/delete/delete[]. Moved
the definitions to separate library
>
>>> @@ -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.
> Does this actually break existing C++ code that overloads operators new/new[]/delete/delete[], or am I missing something? If it does break code, is that "by design" or "by accident"?
No, calls to standard new/new[]/delete/delete[], either replaced or not,
will be substituted, calls to overloaded new/new[]/delete/delete[] will
remain unchanged. Calls to new/new[]/delete/delete[] declared in the
class scope remain unchanged regardless of the signature.
>> 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.
> If the PS3 header<new> has the declarations of the aligned new/delete operators now, why is your patch adding these operators to Clang's mm_alloc.h? Isn't the precedent already there to make sure<new> has these declarations, which is most likely the better answer?
I want the declarations live in the <new> header, but I want the feature
be independent of the library. Adding <new> header to clang leads to
ambiguity. Moved the declarations temporarily to separate header
aligned_new.h. It will not be included often as standard non-placement
declarations are now synthesized by clang. I can make the rest of the
declarations also be synthesized and then there will be no need to
include aligned_new.h at all
>> 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.
> Every type has alignment, so you're saying that every "standard" new/delete/new[]/delete[] expression calls into the aligned version of new/delete/new[]/delete[]. And you're only talking about the global operators new/delete/new[]/delete[] that have one of the forms described in<new>; this replacement doesn't apply for operators with different signatures from those in<new> or for those operators when they're defined at class scope. Is the omission of support for allocation/deallocation functions at class scope intentional? If so, the compiler should emit an error if someone does write one of these operators at class scope, because they're useless (and accepting them is misleading). If not, then we're missing code to support that case.
Calls to new/new[]/delete/delete[] declared in the class scope are all
left unchanged. No errors or warnings are generated. That is how
ppu-lv2-gcc does. Do you want me to change the behavior?
>
> Some more detailed comments to follow, but I'm having a lot of trouble with this feature. I understand that the design of the feature is not really up for debate, because we're implementing an extension that was defined by another compiler, and I can live with that. That said, we need to have a specification of this feature that users and compiler writers alike can use to understand how this feature is supposed to work, because we don't all have access to a copy of ppu-lv2-gcc to act as an oracle for how this feature behaves. The specification needs to go into Clang's language extensions page, so people know it is there and usable:
>
> http://clang.llvm.org/docs/LanguageExtensions.html
Updated LanguageExtensions.html
>
> Now, for the minor comments:
>
> Index: include/clang/Basic/LangOptions.h
> ===================================================================
> --- include/clang/Basic/LangOptions.h (revision 138994)
> +++ include/clang/Basic/LangOptions.h (working copy)
> @@ -140,6 +140,8 @@
>
> unsigned MRTD : 1; // -mrtd calling convention
> unsigned DelayedTemplateParsing : 1; // Delayed template parsing
>
> + unsigned StrictAligned : 1; // Enforce aligned attribute with 'operator new'
> +
> private:
> // We declare multibit enums as unsigned because MSVC insists on making enums
> // signed. Set/Query these values using accessors.
>
> Every LangOptions flag needs to be serialized/de-serialized and checked by the ASTWriter and ASTReader.
>
> Also, StrictAligned needs to be initialized.
Done
>
> Index: include/clang/Driver/CC1Options.td
> ===================================================================
> --- include/clang/Driver/CC1Options.td (revision 138994)
> +++ include/clang/Driver/CC1Options.td (working copy)
> @@ -433,6 +433,8 @@
>
> HelpText<"Disable implicit builtin knowledge of functions">;
> def faltivec : Flag<"-faltivec">,
> HelpText<"Enable AltiVec vector initializer syntax">;
> +def fstrict_aligned : Flag<"-fstrict-aligned">,
> + HelpText<"Enforce aligned attribute with 'operator new'">;
> def fno_access_control : Flag<"-fno-access-control">,
> HelpText<"Disable C++ access control">;
> def fno_assume_sane_operator_new : Flag<"-fno-assume-sane-operator-new">,
>
> This is a -cc1-level flag. I assume that you also want it to work in the Clang driver? You'll need to update the driver, too.
Done
>
> Index: lib/AST/Decl.cpp
> ===================================================================
> --- lib/AST/Decl.cpp (revision 138994)
> +++ lib/AST/Decl.cpp (working copy)
> @@ -1533,7 +1533,31 @@
>
> getIdentifier()->isStr("main");
> }
>
> +bool FunctionDecl::isAlignedSubstitutionOfNewDelete(bool CheckNew,
> + bool CheckNewArr,
> + bool CheckDelete,
> + bool CheckDeleteArr) const {
> +
> + if (!getASTContext().getLangOptions().StrictAligned)
> + return false;
> +
> + IdentifierInfo *II = getDeclName().getAsIdentifierInfo();
> +
> + return (II&& ((CheckNew&& II->isStr("__aligned_new")) ||
> + (CheckNewArr&& II->isStr("__aligned_new_arr")) ||
> + (CheckDelete&& II->isStr("__aligned_delete")) ||
> + (CheckDeleteArr&& II->isStr("__aligned_delete_arr"))));
> +}
>
> isStr() is inefficient. You'll need to cache the IdentifierInfos for these four strings and perform pointer comparisons against II.
>
> I also find this API to be a bit strange… why not call this classifyAlignedSubstitutionOfNewDelete() and return a value of enumeration type that tells us which one it is?
Done
>
> bool FunctionDecl::isReservedGlobalPlacementOperator() const {
> +
> + // calls to new/delete were substituted by calls to it's aligned analogs
> + if (isAlignedSubstitutionOfNewDelete(
> + /*new*/true, /*new[]*/true, /*delete*/true, /*delete[]*/true))
> + return false;
> + //FIXME: maybe add logic for detecting aligned analogs of reserved global
> + // placement operators
>
> Why is this isAlignedSubstitutionOfNewDelete check even necessary? If you're changing the contract of FunctionDecl::isReservedGlobalPlacementOperator() to allow functions whose name isn't a C++ new/delete/new[]/delete[] operator, that's fine, but just return false if
Done
>
> getDeclName().getNameKind() != DeclarationName::CXXOperatorName
>
> @@ -1836,6 +1839,9 @@
> else
> Opts.ObjCXXARCStandardLibrary = (ObjCXXARCStandardLibraryKind)Library;
> }
> +
> + if(Args.hasArg(OPT_fstrict_aligned))
> + Opts.Includes.push_back("mm_malloc.h");
> }
>
> Architecturally, I'm strongly opposed to auto-including headers like this. Either the user should have to include a header manually (e.g.,<new>) or the compiler should synthesize the declarations it needs on demand.
Done
What is with the option name John was talking about? Should i change the
--
Anton
-------------- next part --------------
A non-text attachment was scrubbed...
Name: aligned_new.patch
Type: text/x-diff
Size: 37346 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110920/b18a1944/attachment.patch>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: aligned_new_test.cpp
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110920/b18a1944/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: aligned_new.h
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110920/b18a1944/attachment.h>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: HelperLibs.zip
Type: application/x-zip-compressed
Size: 3913 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110920/b18a1944/attachment.bin>
More information about the cfe-dev
mailing list