[cfe-dev] aligned 'new'
Anton Yartsev
anton.yartsev at gmail.com
Tue Dec 6 06:10:18 PST 2011
Updated the patch.
Please review!
>> 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
>>
--
Anton
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: aligned_new.h
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111206/ff400180/attachment.h>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: aligned_new_test.cpp
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111206/ff400180/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: aligned_new_v1.1.patch
Type: text/x-diff
Size: 28411 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111206/ff400180/attachment.patch>
-------------- 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/20111206/ff400180/attachment.bin>
More information about the cfe-dev
mailing list