[PATCH] Refactor AttributeSet::typeIncompatible to avoid creating new AttributeSets

[Pete Cooper]

> On May 6, 2015, at 3:24 PM, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> Hi Hal
> 
> Thanks for the feedback
>> On May 6, 2015, at 3:15 PM, Hal Finkel <hfinkel at anl.gov <mailto:hfinkel at anl.gov>> wrote:
>> 
>> Hi Pete,
>> 
>> attrbuilder-remove.diff:
Landed with the changes suggested in r236666.
> 
>> 
>> +  /// \brief Remove the specified attributes at the specified index from this
>> 
>> +  /// attribute list. Since attribute lists are immutable, this returns the new
>> 
>> +  /// list.
>> 
>> Since -> Because (if you don't like starting a sentence with because, then reorder it)
> No problem with me.  I’ll make the change.
Also made this change to the other methods in Attribute.h so that they all match.  r236667

...
>> 
>> +#ifndef NDEBUG
>> 
>> +  // FIXME it is not obvious how this should work for alignment.
>> 
>> +  // For now, say we can't pass in alignment, which no current use does.
>> 
>> +  assert(!Attrs.hasAlignmentAttr() && "Attempt to change alignment!");
>> 
>> +#endif
>> 
>> No need to surround the assert with the ifdefs
> Good point.
> 
> I copied that code from here. I’ll clean up the following in another commit.
> 
> AttributeSet AttributeSet::removeAttributes(LLVMContext &C, unsigned Index,
>                                             AttributeSet Attrs) const {
>   if (!pImpl) return AttributeSet();
>   if (!Attrs.pImpl) return *this;
> 
> #ifndef NDEBUG
>   // FIXME it is not obvious how this should work for alignment.
>   // For now, say we can't pass in alignment, which no current use does.
>   assert(!Attrs.hasAttribute(Index, Attribute::Alignment) &&
>          "Attempt to change alignment!");
> #endif
>> 
>> Otherwise, LGTM.
>> 
>> type-incompatible.diff:
And this one was r236668.

Thanks again for the review.

Pete
>> 
>> LGTM.
> Great!  Cheers.
> 
> Will land the patches soon.  Probably tomorrow.
> 
> Thanks,
> Pete
>> 
>> Thanks again,
>> Hal
>> 
>> ----- Original Message -----
>>> From: "Pete Cooper" <peter_cooper at apple.com <mailto:peter_cooper at apple.com>>
>>> To: "Hal Finkel" <hfinkel at anl.gov <mailto:hfinkel at anl.gov>>
>>> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>>
>>> Sent: Wednesday, April 22, 2015 8:12:14 PM
>>> Subject: [PATCH] Refactor AttributeSet::typeIncompatible to avoid creating new AttributeSets
>>> 
>>> Hi Hal
>>> 
>>> These 2 patches change the method typeIncompatible to avoid creating
>>> temporary AttributeSets.  This actually shows up as a fraction of a
>>> percent of compile time because the IR verify pass calls
>>> typeIncompatible to check the attributes on calls.
>>> 
>>> The first patch is attrbuilder-remove.diff which adds methods to the
>>> AttrBuilder itself to remove another AttrBuilder and to check if
>>> there is any overlap between 2 AttrBuilders.
>>> 
>>> The next patch - type-incompatible.diff - actually uses this new
>>> functionality to change that method to return an AttrBuilder and
>>> rewrite the few calls to it to work on an AttrBuilder instead of
>>> AttributeSet.
>>> 
>>> This is only the start of the work I’d like to do on AttributeSets.
>>> There’s more to be done to reduce the number of sets we allocate
>>> (my current thinking involves adding an AttributeSetRef which
>>> references an index in to a set, and is safe because of the
>>> immutability of AttributeSet).  Let me know if you have anything
>>> else in mind.
>>> 
>>> Cheers,
>>> Pete
>>> 
>>> 
>> 
>> -- 
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> 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/20150506/62c4ba10/attachment.html>