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

[Pete Cooper]

Hi Hal

Thanks for the feedback
> On May 6, 2015, at 3:15 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> Hi Pete,
> 
> attrbuilder-remove.diff:
> 
> +  /// \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.
> 
> +#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:
> 
> 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>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: "llvm-commits" <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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150506/daaed6b9/attachment.html>