[PATCH] D14305: [IR] Add a `data_operand` abstraction

[Philip Reames via llvm-commits]

reames added a comment.

Only minor code comments left.  Only remaining piece is a couple of stale comments.

However, I'm somewhat questioning whether this is the right direction.  See my inline comment per why.


================
Comment at: include/llvm/IR/CallSite.h:187
@@ +186,3 @@
+  unsigned data_operands_size() const {
+    return unsigned(data_operands_end() - data_operands_begin());
+  }
----------------
use std::distance here.

================
Comment at: include/llvm/IR/CallSite.h:273
@@ +272,3 @@
+  /// \brief Return true if the data operand (i.e. call / invoke argument or
+  /// bundle operand) at index \p i has the attribute \p A.
+  bool dataOperandHasImpliedAttr(unsigned i, Attribute::AttrKind A) const {
----------------
Doesn't distinguish between "has an attribute" and "implies an attribute".  Specifically, operand bundles don't yet have per operand attributes, but do imply attributes based on their tag type.  (Based on what you've said previously.)

================
Comment at: include/llvm/IR/InstrTypes.h:1130
@@ +1129,3 @@
+  bool operandsHaveAttr(Attribute::AttrKind A) const {
+    // Conservative answer:  no operands have any attributes.
+    return false;
----------------
Given this, I'm somewhat questioning the need for this entire change.  How horrible would it be to just make each place which tries to visit call site arguments have a check for whether the call has any bundle and then bail?  This sorta feels like unnecessarily generality.  

================
Comment at: include/llvm/IR/Instructions.h:3500
@@ +3499,3 @@
+  ///
+  /// To check an attribute on the return value, set \p i to 0.  To check an
+  /// attribute on the operand at index k (invoke argument, or bundle operand),
----------------
Stale comment needs to updated as above.


http://reviews.llvm.org/D14305