[llvm] r239428 - Make MCSymbol::Name be a union of uint64_t and a pointer.
Pete Cooper
peter_cooper at apple.com
Tue Jun 9 14:02:50 PDT 2015
> On Jun 9, 2015, at 1:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Jun 9, 2015 at 1:52 PM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
> Thanks for the feedback. Comments below, based on r239429 which i just pushed.
>> On Jun 9, 2015, at 1:10 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>
>>
>>
>> On Tue, Jun 9, 2015 at 12:56 PM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
>> Author: pete
>> Date: Tue Jun 9 14:56:05 2015
>> New Revision: 239428
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=239428&view=rev <http://llvm.org/viewvc/llvm-project?rev=239428&view=rev>
>> Log:
>> Make MCSymbol::Name be a union of uint64_t and a pointer.
>>
>> This should hopefully fix the 32-bit bots which were allocating space for a pointer
>> but needed to be aligned to 64-bits.
>>
>> Now we allocate enough space for a uint64_t and a pointer and cast to the appropriate storage
>>
>> This kind fo complicates the rest of the MCSymbol code. A few options:
>>
>> have getNameEntry return the NameEntry reference as before (do the ".NameEntry" inside getNameEntry rather than in both the callers).
> This is done.
>>
>> Possibly just do the interesting uint64_t stuff inside operator new - though I suppose you'd still need some magic in getNameEntry that would look similar.
> I thought about this, but because it was handled in getNameEntry() too, I went for the union solution so that at least everything is based on the union size and storage.
>>
>> & the alignment assertion could probably be a static_assert? (might need to check if alignOf is constexpr or if we have a trait/constexpr form of it)
> Turns out constexpr is still not used much in LLVM. We have a macro for it. I guess MSVC doesn’t support it fully on our version. I’ve added a FIXME so that we can handle this later.
>
> Seems we have at least one example of a static assertion for alignment:
>
> include/llvm/Support/ArrayRecycler.h: static_assert(Align >= AlignOf<FreeList>::Alignment, "Object underaligned");
>
> So if you use the llvm::AlignOf trait instead of llvm::alignOf function, you should be able to make it a static instead of a dynamic assertion. Maybe?
Yeah, that worked. Thanks! r239431.
(will keep an eye on the bots just in case MSVC complains, but I assume it’ll be ok given the precedence you found)
>
>> Oh, side note (should've caught this in review): you could have getNameEntry non-const call getNameEntry const and just cast away the constness on the resulting reference - to reduce some of that code duplication.
> I’ve done this. I had to cast away the const-ness and make the const method call the non-const one. This was due to the const-ness of the result. Basically, this solution is a single const_cast, but the other way would have been 2.
>
> Thanks for the help!
> Pete
>
>>
>>
>> Modified:
>> llvm/trunk/include/llvm/MC/MCSymbol.h
>> llvm/trunk/lib/MC/MCSymbol.cpp
>>
>> Modified: llvm/trunk/include/llvm/MC/MCSymbol.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCSymbol.h?rev=239428&r1=239427&r2=239428&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCSymbol.h?rev=239428&r1=239427&r2=239428&view=diff>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/MC/MCSymbol.h (original)
>> +++ llvm/trunk/include/llvm/MC/MCSymbol.h Tue Jun 9 14:56:05 2015
>> @@ -120,20 +120,29 @@ protected: // MCContext creates and uniq
>> friend class MCExpr;
>> friend class MCContext;
>>
>> - typedef const StringMapEntry<bool> NameEntryTy;
>> - MCSymbol(SymbolKind Kind, NameEntryTy *Name, bool isTemporary)
>> + /// \brief The name for a symbol.
>> + /// MCSymbol contains a uint64_t so is probably aligned to 8. On a 32-bit
>> + /// system, the name is a pointer so isn't going to satisfy the 8 byte
>> + /// alignment of uint64_t. Account for that here.
>> + typedef union {
>> + const StringMapEntry<bool> *NameEntry;
>> + uint64_t AlignmentPadding;
>> + } NameEntryStorageTy;
>> +
>> + MCSymbol(SymbolKind Kind, const StringMapEntry<bool> *Name, bool isTemporary)
>> : Value(nullptr), IsTemporary(isTemporary),
>> IsRedefinable(false), IsUsed(false), IsRegistered(false),
>> IsExternal(false), IsPrivateExtern(false), HasName(!!Name),
>> Kind(Kind) {
>> Offset = 0;
>> if (Name)
>> - getNameEntryPtr() = Name;
>> + getNameEntryPtr().NameEntry = Name;
>> }
>>
>> // Provide custom new/delete as we will only allocate space for a name
>> // if we need one.
>> - void *operator new(size_t s, NameEntryTy *Name, MCContext &Ctx);
>> + void *operator new(size_t s, const StringMapEntry<bool> *Name,
>> + MCContext &Ctx);
>>
>> private:
>>
>> @@ -160,14 +169,14 @@ private:
>> }
>>
>> /// \brief Get a reference to the name field. Requires that we have a name
>> - NameEntryTy *&getNameEntryPtr() {
>> + NameEntryStorageTy &getNameEntryPtr() {
>> assert(HasName && "Name is required");
>> - NameEntryTy **Name = reinterpret_cast<NameEntryTy **>(this);
>> + NameEntryStorageTy *Name = reinterpret_cast<NameEntryStorageTy *>(this);
>> return *(Name - 1);
>> }
>> - NameEntryTy *const &getNameEntryPtr() const {
>> + const NameEntryStorageTy &getNameEntryPtr() const {
>> assert(HasName && "Name is required");
>> - NameEntryTy *const *Name = reinterpret_cast<NameEntryTy *const *>(this);
>> + const auto *Name = reinterpret_cast<const NameEntryStorageTy *>(this);
>> return *(Name - 1);
>> }
>>
>> @@ -177,7 +186,7 @@ public:
>> if (!HasName)
>> return StringRef();
>>
>> - return getNameEntryPtr()->first();
>> + return getNameEntryPtr().NameEntry->first();
>> }
>>
>> bool isRegistered() const { return IsRegistered; }
>>
>> Modified: llvm/trunk/lib/MC/MCSymbol.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCSymbol.cpp?rev=239428&r1=239427&r2=239428&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCSymbol.cpp?rev=239428&r1=239427&r2=239428&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/MC/MCSymbol.cpp (original)
>> +++ llvm/trunk/lib/MC/MCSymbol.cpp Tue Jun 9 14:56:05 2015
>> @@ -19,17 +19,20 @@ using namespace llvm;
>> // Sentinel value for the absolute pseudo section.
>> MCSection *MCSymbol::AbsolutePseudoSection = reinterpret_cast<MCSection *>(1);
>>
>> -void *MCSymbol::operator new(size_t s, NameEntryTy *Name, MCContext &Ctx) {
>> - size_t Size = s + (Name ? sizeof(Name) : 0);
>> +void *MCSymbol::operator new(size_t s, const StringMapEntry<bool> *Name,
>> + MCContext &Ctx) {
>> + // We may need more space for a Name to account for alignment. So allocate
>> + // space for the storage type and not the name pointer.
>> + size_t Size = s + (Name ? sizeof(NameEntryStorageTy) : 0);
>>
>> // For safety, ensure that the alignment of a pointer is enough for an
>> // MCSymbol. This also ensures we don't need padding between the name and
>> // symbol.
>> - assert(alignOf<MCSymbol>() <= alignOf<NameEntryTy *>() &&
>> + assert(alignOf<MCSymbol>() <= alignOf<NameEntryStorageTy>() &&
>> "Bad alignment of MCSymbol");
>> - void *Storage = Ctx.allocate(Size, alignOf<NameEntryTy *>());
>> - NameEntryTy **Start = static_cast<NameEntryTy**>(Storage);
>> - NameEntryTy **End = Start + (Name ? 1 : 0);
>> + void *Storage = Ctx.allocate(Size, alignOf<NameEntryStorageTy>());
>> + NameEntryStorageTy *Start = static_cast<NameEntryStorageTy*>(Storage);
>> + NameEntryStorageTy *End = Start + (Name ? 1 : 0);
>> return End;
>> }
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150609/e1b88740/attachment.html>
More information about the llvm-commits
mailing list