[PATCH] D15085: [AttributeSet] Overload AttributeSet::addAttribute to reduce compile time

[Akira Hatanaka via llvm-commits]

ahatanak added inline comments.

================
Comment at: lib/IR/Attributes.cpp:775
@@ +774,3 @@
+                                        ArrayRef<unsigned> Indices,
+                                        Attribute A) const {
+  unsigned I = 0, E = pImpl ? pImpl->getNumAttributes() : 0;
----------------
pete wrote:
> ahatanak wrote:
> > pete wrote:
> > > I didn't know we could actually pass around 'Attribute' which would wrap up enum/integer/string attributes.  Unfortunately this could lead to a slight difference in behavior compared to the old way of adding attributes.  The difference being that you don't have this code in your implementation:
> > > 
> > > #ifndef NDEBUG
> > >   // FIXME it is not obvious how this should work for alignment. For now, say
> > >   // we can't change a known alignment.
> > >   unsigned OldAlign = getParamAlignment(Index);
> > >   unsigned NewAlign = Attrs.getParamAlignment(Index);
> > >   assert((!OldAlign || !NewAlign || OldAlign == NewAlign) &&
> > >          "Attempt to change alignment!");
> > > #endif
> > > 
> > > I imagine you could add that to the appropriate conditional in your loop, although i'm not sure if we want to or not.  Up to you, but just wanted to point it out.
> > > 
> > > Another solution is to change your API from 'Attribute' to 'Attribute::AttrKind' and so only accept enum attributes.  I kinda like the generic API which can take any attributes, but the enum has the advantage of not worrying about the alignment issue (which FYI is totally out of date anyway as we have more integer attributes than just alignment, and none of the others are checked here)
> > I wonder if we can remove the assert in AttributeSet::addAttributes and just allow it to override the old alignment using the alignment in Attrs. This is consistent with the way it handles string attributes: if there is an attribute "some_str_attr"="true" in Attrs, it will change the value of attribute "some_str_attr"="false". Also, as you've pointed out, we don't check other integer attributes here, so if we are going to keep the assert we should make changes to check those attributes too.
> Yeah, removing that assert is probably best.  If someone did trigger the assert, then I imagine all that would happen is they would change the call from AS.addAttribute(..., ::Align) to AS.removeAttribute(..., ::Align).addAttribute(..., ::Align) which would match the behavior you describe anyway.
> 
> I guess the issue might be that its likely incorrect to lower the alignment.  Other code could have applied optimizations based on a given alignment so lowering it would invalidate those optimizations.  But then again, thats true for pretty much all attributes anyway.   You couldn't go and remove a readonly for example as someone may have used the presence of readonly to move a store around.  The point is that we don't assert on readonly or anything else, so seems weird to assert on alignment.
> 
> BTW, I think there's a few places which assert on alignment.  I think AttrBuilder has some too, so certainly worth looking in to that in a follow up patch.
OK, I'll take care of this in follow-up patches.

In the short term, I might just add a check similar to the one in AttributeSet::addAttributes, but it seems to me that the users of those functions should be responsible for making sure adding or removing attributes doesn't invalidate optimizations that have already been applied.


http://reviews.llvm.org/D15085