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

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: include/llvm/IR/CallSite.h:172
@@ +171,3 @@
+
+  IterTy data_op_begin() const {
+    assert(getInstruction() && "Not a call or invoke instruction!");
----------------
I would just call this data_operands_X.  Slightly longer, but clearer.

================
Comment at: include/llvm/IR/CallSite.h:173
@@ +172,3 @@
+  IterTy data_op_begin() const {
+    assert(getInstruction() && "Not a call or invoke instruction!");
+    return (*this)->op_begin();
----------------
Why have this assert here and no where else?

================
Comment at: include/llvm/IR/CallSite.h:268
@@ +267,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 {
----------------
Stale comment?  

================
Comment at: include/llvm/IR/CallSite.h:381
@@ +380,3 @@
+  /// @brief Determine whether this data operand is passed by value.
+  bool isByValArgument(unsigned OpNo) const {
+    return dataOperandHasImpliedAttr(OpNo + 1, Attribute::ByVal);
----------------
These next couple seem specific to arguments.  I'd be tempted to make that explicit and use the argument version so that we get proper asserts.

================
Comment at: include/llvm/IR/CallSite.h:393
@@ +392,3 @@
+  bool isByValOrInAllocaArgument(unsigned OpNo) const {
+    return dataOperandHasImpliedAttr(OpNo + 1, Attribute::ByVal) ||
+           dataOperandHasImpliedAttr(OpNo + 1, Attribute::InAlloca);
----------------
Please just reuse the two other functions.

================
Comment at: include/llvm/IR/CallSite.h:397
@@ -370,3 +396,3 @@
 
-  /// @brief Determine if there are is an inalloca argument.  Only the last
+  /// @brief Determine if there are is an inalloca argument.  Only the last call
   /// argument can have the inalloca attribute.
----------------
I don't think you need the clarification here any more.

================
Comment at: include/llvm/IR/InstrTypes.h:1237
@@ -1215,6 +1236,3 @@
     assert(Index < getNumOperandBundles() && "Index out of bounds!");
-    auto *BOI = bundle_op_info_begin() + Index;
-    auto op_begin = static_cast<const InstrTy *>(this)->op_begin();
-    ArrayRef<Use> Inputs(op_begin + BOI->Begin, op_begin + BOI->End);
-    return OperandBundleUse(BOI->Tag->getKey(), Inputs);
+    return operandBundleFromBundleOpInfo(bundle_op_info_begin()[Index]);
   }
----------------
This looks like a possibly unrelated change?

================
Comment at: include/llvm/IR/Instructions.h:1613
@@ +1612,3 @@
+  ///
+  /// To check an attribute on the return value, set \p i to 0.  To check an
+  /// attribute on the operand at index k (call argument, or bundle operand),
----------------
No!  Not '0', Attribute::ReturnIndex!  Don't encourage magic constants.

================
Comment at: include/llvm/IR/Instructions.h:1614
@@ +1613,3 @@
+  /// To check an attribute on the return value, set \p i to 0.  To check an
+  /// attribute on the operand at index k (call argument, or bundle operand),
+  /// set \p i to k + 1.
----------------
I get what you're going for here, but the wording is really confusing.  *Which* index?  

For arguments, I'd just say: ZeroBasedArgNo + 1
For bundle operands, how do you even handle this?  Are you essentially extending ArgNo?  It's not the underlying operand number right?

================
Comment at: lib/IR/Instructions.cpp:350
@@ +349,3 @@
+
+  if (i < (getNumArgOperands() + 1))
+    return paramHasAttr(i, A);
----------------
This would also catch FunctionIndex.  Intentional?


http://reviews.llvm.org/D14305