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

[Pete Cooper via llvm-commits]

pete added a subscriber: pete.
pete accepted this revision.
pete added a reviewer: pete.
pete added a comment.
This revision is now accepted and ready to land.

A few nitpicks, but honestly this is great.  So LGTM.

Many of the nitpicks are up to you if you even want to fix them.  I don't think any block this from going in.

Thanks for doing this!


================
Comment at: include/llvm/IR/Attributes.h:230
@@ -229,1 +229,3 @@
 
+  /// \brief Add an attribute to the attribute set at the given indices. Because
+  /// attribute sets are immutable, this returns a new set.
----------------
Nitpicking, but we don't put \brief any more.  Although you were just matching the style of th rest of the file, so probably fine to leave it here.

================
Comment at: lib/IR/Attributes.cpp:775
@@ +774,3 @@
+                                        ArrayRef<unsigned> Indices,
+                                        Attribute A) const {
+  unsigned I = 0, E = pImpl ? pImpl->getNumAttributes() : 0;
----------------
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)

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

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:323
@@ -324,2 +322,3 @@
+      Indices.push_back(ArgNo + 1);
       Changed = true;
     }
----------------
Similarly here.

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:330
@@ -329,1 +329,3 @@
 
+  if (!Indices.empty()) {
+    AttributeSet AS = CS.getAttributes();
----------------
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.


http://reviews.llvm.org/D15085