[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:
> 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.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1952
@@ -1953,2 +1951,3 @@
+      Indices.push_back(ArgNo + 1);
       Changed = true;
     }
----------------
pete wrote:
> Again nitpicking, but I would move the 'Changed = true' in to the 'if (!Indices.empty()) {' conditional as thats where you apply the changes.
Will fix

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:330
@@ -329,1 +329,3 @@
 
+  if (!Indices.empty()) {
+    AttributeSet AS = CS.getAttributes();
----------------
pete wrote:
> And this conditional can be changed to be 'if (Indices.empty()) return false'.  Then you can follow it with what was in the conditional and a 'return true' as no need for the 'bool Changed' any more in this case.
Will fix.


http://reviews.llvm.org/D15085