[IR] Add a `data_operand` abstraction

[sanjoy]

Summary

Data operands of a call or invoke consist of the call arguments, and
the bundle operands associated with the call (or invoke)
instruction. The motivation for this change is that we'd like to be
able to query "argument attributes" like readonly and nocapture
for bundle operands naturally.

This change also provides a conservative "implementation" for these
attributes for any bundle operand, and an extension point for future
work.

[reames]

I strongly dislike the use of parameter to refer to anything other than the call arguments. Arguments and Parameters are so strongly associated in the literature that using them for a slightly different purpose would be actively confusing.

Having an abstraction over both makes sense, but this is definitely the wrong name.

[sanjoy]

In D14305#280571, @reames wrote:

I strongly dislike the use of parameter to refer to anything other than the call arguments. Arguments and Parameters are so strongly associated in the literature that using them for a slightly different purpose would be actively confusing.

Having an abstraction over both makes sense, but this is definitely the wrong name.

Fair enough -- do you have a suggestion? The alternatives I can think of, input and operand, are both too generic.

[sanjoy]

• rename "parameters" to "data operands"

[sanjoy]

• comments and whitespace fixes

[reames]

include/llvm/IR/CallSite.h

172

I would just call this data_operands_X. Slightly longer, but clearer.

173

Why have this assert here and no where else?

268

Stale comment?

381

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.

393

Please just reuse the two other functions.

397

I don't think you need the clarification here any more.

include/llvm/IR/InstrTypes.h

1237

This looks like a possibly unrelated change?

include/llvm/IR/Instructions.h

1613

No! Not '0', Attribute::ReturnIndex! Don't encourage magic constants.

1614

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?

lib/IR/Instructions.cpp

350

This would also catch FunctionIndex. Intentional?

[sanjoy]

include/llvm/IR/CallSite.h

173

These iterators are basically just copied over from the arg_xx iterators. :)

I'll add the assert to each of the iterator accessors.

268

Why stale?

lib/IR/Instructions.cpp

350

It won't since the index is unsigned.

[sanjoy]

• address review comments. Note: some of the NFC refactoring has been commited separately

[sanjoy]

include/llvm/IR/Instructions.h

1614

I've added a clarifying comment.

[reames]

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.

include/llvm/IR/CallSite.h

187

use std::distance here.

268

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.)

include/llvm/IR/InstrTypes.h

1130

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.

include/llvm/IR/Instructions.h

3497

Stale comment needs to updated as above.

[sanjoy]

include/llvm/IR/InstrTypes.h

1130

We will soon have nocapture and readonly for values in the "deopt" operand bundle here.

[reames]

LGTM w/previous minor comments applied.

include/llvm/IR/InstrTypes.h

1130

Ah. Duh. :)

include/llvm/IR/CallSite.h

public:
 
   IterTy arg_end() const { return (*this)->op_end() - getArgumentEndOffset(); }
   iterator_range<IterTy> args() const {
     return iterator_range<IterTy>(arg_begin(), arg_end());
   }
   bool arg_empty() const { return arg_end() == arg_begin(); }
   unsigned arg_size() const { return unsigned(arg_end() - arg_begin()); }
 
+  /// data_op_begin/data_op_end - Return iterators iterating over the call /
+  /// invoke argument list and bundle operands.  For invokes, this is the set of
+  /// instruction operands except the invoke target and the two successor
+  /// blocks; and for calls this is the set of instruction operands except the
+  /// call target.
+
+  IterTy data_op_begin() const {
+    assert(getInstruction() && "Not a call or invoke instruction!");
+    return (*this)->op_begin();
+  }
+  IterTy data_op_end() const { return (*this)->op_end() - (isCall() ? 1 : 3); }
+  iterator_range<IterTy> data_ops() const {
+    return iterator_range<IterTy>(data_op_begin(), data_op_end());
+  }
+  bool data_op_empty() const { return data_op_end() == data_op_begin(); }
+  unsigned data_op_size() const {
+    return unsigned(data_op_end() - data_op_begin());
+  }
+
   /// getType - Return the type of the instruction that generated this call site
   ///
   Type *getType() const { return (*this)->getType(); }
 
   /// getCaller - Return the caller function for this call site
   ///
   FunTy *getCaller() const { return (*this)->getParent()->getParent(); }
 
   /// \brief Tests if this call site must be tail call optimized.  Only a
   /// CallInst can be tail call optimized.
   bool isMustTailCall() const {
bool hasFnAttr(Attribute::AttrKind A) const {
     CALLSITE_DELEGATE_GETTER(hasFnAttr(A));
   }
 
   /// \brief Return true if the call or the callee has the given attribute.
   bool paramHasAttr(unsigned i, Attribute::AttrKind A) const {
     CALLSITE_DELEGATE_GETTER(paramHasAttr(i, A));
   }
 
+  /// \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 {
+    CALLSITE_DELEGATE_GETTER(dataOperandHasImpliedAttr(i, A));
+  }
+
   /// @brief Extract the alignment for a call or parameter (0=unknown).
   uint16_t getParamAlignment(uint16_t i) const {
     CALLSITE_DELEGATE_GETTER(getParamAlignment(i));
   }
 
   /// @brief Extract the number of dereferenceable bytes for a call or
   /// parameter (0=unknown).
   uint64_t getDereferenceableBytes(uint16_t i) const {
#define CALLSITE_DELEGATE_SETTER(METHOD) \
 
   Optional<OperandBundleUse> getOperandBundle(StringRef Name) const {
     CALLSITE_DELEGATE_GETTER(getOperandBundle(Name));
   }
 
 #undef CALLSITE_DELEGATE_GETTER
 #undef CALLSITE_DELEGATE_SETTER
 
-  /// @brief Determine whether this argument is not captured.
-  bool doesNotCapture(unsigned ArgNo) const {
-    return paramHasAttr(ArgNo + 1, Attribute::NoCapture);
+  /// @brief Determine whether this data operand is not captured.
+  bool doesNotCapture(unsigned OpNo) const {
+    return dataOperandHasImpliedAttr(OpNo + 1, Attribute::NoCapture);
   }
 
-  /// @brief Determine whether this argument is passed by value.
-  bool isByValArgument(unsigned ArgNo) const {
-    return paramHasAttr(ArgNo + 1, Attribute::ByVal);
+  /// @brief Determine whether this data operand is passed by value.
+  bool isByValArgument(unsigned OpNo) const {
+    return dataOperandHasImpliedAttr(OpNo + 1, Attribute::ByVal);
   }
 
-  /// @brief Determine whether this argument is passed in an alloca.
-  bool isInAllocaArgument(unsigned ArgNo) const {
-    return paramHasAttr(ArgNo + 1, Attribute::InAlloca);
+  /// @brief Determine whether this data operand is passed in an alloca.
+  bool isInAllocaArgument(unsigned OpNo) const {
+    return dataOperandHasImpliedAttr(OpNo + 1, Attribute::InAlloca);
   }
 
-  /// @brief Determine whether this argument is passed by value or in an alloca.
-  bool isByValOrInAllocaArgument(unsigned ArgNo) const {
-    return paramHasAttr(ArgNo + 1, Attribute::ByVal) ||
-           paramHasAttr(ArgNo + 1, Attribute::InAlloca);
+  /// @brief Determine whether this data operand is passed by value or in an
+  /// alloca.
+  bool isByValOrInAllocaArgument(unsigned OpNo) const {
+    return dataOperandHasImpliedAttr(OpNo + 1, Attribute::ByVal) ||
+           dataOperandHasImpliedAttr(OpNo + 1, Attribute::InAlloca);
   }
 
-  /// @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.
   bool hasInAllocaArgument() const {
     return paramHasAttr(arg_size(), Attribute::InAlloca);
   }
 
-  bool doesNotAccessMemory(unsigned ArgNo) const {
-    return paramHasAttr(ArgNo + 1, Attribute::ReadNone);
+  bool doesNotAccessMemory(unsigned OpNo) const {
+    return dataOperandHasImpliedAttr(OpNo + 1, Attribute::ReadNone);
   }
 
-  bool onlyReadsMemory(unsigned ArgNo) const {
-    return paramHasAttr(ArgNo + 1, Attribute::ReadOnly) ||
-           paramHasAttr(ArgNo + 1, Attribute::ReadNone);
+  bool onlyReadsMemory(unsigned OpNo) const {
+    return dataOperandHasImpliedAttr(OpNo + 1, Attribute::ReadOnly) ||
+           dataOperandHasImpliedAttr(OpNo + 1, Attribute::ReadNone);
   }
 
   /// @brief Return true if the return value is known to be not null.
   /// This may be because it has the nonnull attribute, or because at least
   /// one byte is dereferenceable and the pointer is in addrspace(0).
   bool isReturnNonNull() const {
     if (paramHasAttr(0, Attribute::NonNull))
       return true;

include/llvm/IR/InstrTypes.h

 /// around by value.
 struct OperandBundleUse {
   StringRef Tag;
   ArrayRef<Use> Inputs;
 
   OperandBundleUse() {}
   explicit OperandBundleUse(StringRef Tag, ArrayRef<Use> Inputs)
       : Tag(Tag), Inputs(Inputs) {}
+
+  /// \brief Return true if all the operands in this operand bundle have the
+  /// attribute A.
+  ///
+  /// Currently there is no way to have attributes on operand bundles differ on
+  /// a per operand granularity.
+  bool operandsHaveAttr(Attribute::AttrKind A) const {
+    // Conservative answer:  no operands have any attributes.
+    return false;
+  };
 };
 
 /// \brief A container for an operand bundle being viewed as a set of values
 /// rather than a set of uses.
 ///
 /// Unlike OperandBundleUse, OperandBundleDefT owns the memory it carries, and
 /// so it is possible to create and pass around "self-contained" instances of
 /// OperandBundleDef and ConstOperandBundleDef.
public:
   /// \brief Return the number of operand bundles associated with this User.
   unsigned getNumOperandBundles() const {
     return std::distance(bundle_op_info_begin(), bundle_op_info_end());
   }
 
   /// \brief Return true if this User has any operand bundles.
   bool hasOperandBundles() const { return getNumOperandBundles() != 0; }
 
+  /// \brief Return the index of the first bundle operand in the Use array.
+  unsigned getBundleOperandsStartIndex() const {
+    assert(hasOperandBundles() && "Don't call otherwise!");
+    return bundle_op_info_begin()->Begin;
+  }
+
+  /// \brief Return the index of the last bundle operand in the Use array.
+  unsigned getBundleOperandsEndIndex() const {
+    assert(hasOperandBundles() && "Don't call otherwise!");
+    return bundle_op_info_end()[-1].End;
+  }
+
   /// \brief Return the total number operands (not operand bundles) used by
   /// every operand bundle in this OperandBundleUser.
   unsigned getNumTotalBundleOperands() const {
     if (!hasOperandBundles())
       return 0;
 
-    auto *Begin = bundle_op_info_begin();
-    auto *Back = bundle_op_info_end() - 1;
-
-    assert(Begin <= Back && "hasOperandBundles() returned true!");
+    unsigned Begin = getBundleOperandsStartIndex();
+    unsigned End = getBundleOperandsEndIndex();
 
-    return Back->End - Begin->Begin;
+    assert(Begin <= End && "Should be!");
+    return End - Begin;
   }
 
   /// \brief Return the operand bundle at a specific index.
   OperandBundleUse getOperandBundle(unsigned Index) const {
     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]);
   }
 
   /// \brief Return the number of operand bundles with the tag Name attached to
   /// this instruction.
   unsigned countOperandBundlesOfType(StringRef Name) const {
     unsigned Count = 0;
     for (unsigned i = 0, e = getNumOperandBundles(); i != e; ++i)
       if (getOperandBundle(i).Tag == Name)
for (unsigned i = 0, e = getNumOperandBundles(); i != e; ++i) {
       OperandBundleUse U = getOperandBundle(i);
       if (U.Tag == Name)
         return U;
     }
 
     return None;
   }
 
+  /// \brief Return the operand bundle for the operand at index OpIdx.
+  ///
+  /// It is an error to call this with an OpIdx that does not correspond to an
+  /// bundle operand.
+  OperandBundleUse getOperandBundleForOperand(unsigned OpIdx) const {
+    for (auto &BOI : bundle_op_infos())
+      if (BOI.Begin <= OpIdx && OpIdx < BOI.End)
+        return operandBundleFromBundleOpInfo(BOI);
+
+    llvm_unreachable("Did not find operand bundle for operand!");
+  }
+
   /// \brief Return true if this operand bundle user has operand bundles that
   /// may read from the heap.
   bool hasReadingOperandBundles() const {
     // Implementation note: this is a conservative implementation of operand
     // bundle semantics, where *any* operand bundle forces a callsite to be at
     // least readonly.
     return hasOperandBundles();
   }
struct BundleOpInfo {
     /// bundle starts.
     uint32_t Begin;
 
     /// \brief The index in the Use& vector where operands for this operand
     /// bundle ends.
     uint32_t End;
   };
 
+  /// \brief Simple helper function to map a BundleOpInfo to an
+  /// OperandBundleUse.
+  OperandBundleUse
+  operandBundleFromBundleOpInfo(const BundleOpInfo &BOI) const {
+    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);
+  }
+
   typedef BundleOpInfo *bundle_op_iterator;
   typedef const BundleOpInfo *const_bundle_op_iterator;
 
   /// \brief Return the start of the list of BundleOpInfo instances associated
   /// with this OperandBundleUser.
   bundle_op_iterator bundle_op_info_begin() {
     if (!static_cast<InstrTy *>(this)->hasDescriptor())
       return nullptr;

include/llvm/IR/Instructions.h

public:
   /// \brief Determine whether this call has the given attribute.
   bool hasFnAttr(StringRef A) const {
     return hasFnAttrImpl(A);
   }
 
   /// \brief Determine whether the call or the callee has the given attributes.
   bool paramHasAttr(unsigned i, Attribute::AttrKind A) const;
 
+  /// \brief Return true if the data operand at index \p i has the attribute \p
+  /// A.
+  ///
+  /// Data operands include call arguments and values used in operand bundles,
+  /// but does not include the callee operand.  This routine dispatches to the
+  /// underlying AttributeList or the OperandBundleUser as appropriate.
+  ///
+  /// 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.
+  bool dataOperandHasImpliedAttr(unsigned i, Attribute::AttrKind A) const;
+
   /// \brief Extract the alignment for a call or parameter (0=unknown).
   unsigned getParamAlignment(unsigned i) const {
     return AttributeList.getParamAlignment(i);
   }
 
   /// \brief Extract the number of dereferenceable bytes for a call or
   /// parameter (0=unknown).
   uint64_t getDereferenceableBytes(unsigned i) const {
bool hasFnAttr(Attribute::AttrKind A) const {
     assert(A != Attribute::NoBuiltin &&
            "Use CallInst::isNoBuiltin() to check for Attribute::NoBuiltin");
     return hasFnAttrImpl(A);
   }
 
   /// \brief Determine whether the call or the callee has the given attributes.
   bool paramHasAttr(unsigned i, Attribute::AttrKind A) const;
 
+  /// \brief Return true if the data operand at index \p i has the attribute \p
+  /// A.
+  ///
+  /// Data operands include invoke arguments and values used in operand bundles,
+  /// but does not include the invokee operand, or the two successor blocks.
+  /// This routine dispatches to the underlying AttributeList or the
+  /// OperandBundleUser as appropriate.
+  ///
+  /// 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),
+  /// set \p i to k + 1.
+  bool dataOperandHasImpliedAttr(unsigned i, Attribute::AttrKind A) const;
+
   /// \brief Extract the alignment for a call or parameter (0=unknown).
   unsigned getParamAlignment(unsigned i) const {
     return AttributeList.getParamAlignment(i);
   }
 
   /// \brief Extract the number of dereferenceable bytes for a call or
   /// parameter (0=unknown).
   uint64_t getDereferenceableBytes(unsigned i) const {

lib/IR/Instructions.cpp

bool CallInst::paramHasAttr(unsigned i, Attribute::AttrKind A) const {
 
   if (AttributeList.hasAttribute(i, A))
     return true;
   if (const Function *F = getCalledFunction())
     return F->getAttributes().hasAttribute(i, A);
   return false;
 }
 
+bool CallInst::dataOperandHasImpliedAttr(unsigned i,
+                                         Attribute::AttrKind A) const {
+
+  // The attribute A can either be directly specified, if the operand in
+  // question is a call argument; or be indirectly implied by the kind of its
+  // containing operand bundle, if the operand is a bundle operand.
+
+  if (i < (getNumArgOperands() + 1))
+    return paramHasAttr(i, A);
+
+  assert(hasOperandBundles() && i >= (getBundleOperandsStartIndex() + 1) &&
+         "Must be either a call argument or an operand bundle!");
+  return getOperandBundleForOperand(i - 1).operandsHaveAttr(A);
+}
+
 /// IsConstantOne - Return true only if val is constant int 1
 static bool IsConstantOne(Value *val) {
   assert(val && "IsConstantOne does not work with nullptr val");
   const ConstantInt *CVal = dyn_cast<ConstantInt>(val);
   return CVal && CVal->isOne();
 }
 
 static Instruction *createMalloc(Instruction *InsertBefore,
bool InvokeInst::paramHasAttr(unsigned i, Attribute::AttrKind A) const {
 
   if (AttributeList.hasAttribute(i, A))
     return true;
   if (const Function *F = getCalledFunction())
     return F->getAttributes().hasAttribute(i, A);
   return false;
 }
 
+bool InvokeInst::dataOperandHasImpliedAttr(unsigned i,
+                                           Attribute::AttrKind A) const {
+  // The attribute A can either be directly specified, if the operand in
+  // question is an invoke argument; or be indirectly implied by the kind of its
+  // containing operand bundle, if the operand is a bundle operand.
+
+  if (i < (getNumArgOperands() + 1))
+    return paramHasAttr(i, A);
+
+  assert(hasOperandBundles() && i >= (getBundleOperandsStartIndex() + 1) &&
+         "Must be either an invoke argument or an operand bundle!");
+  return getOperandBundleForOperand(i - 1).operandsHaveAttr(A);
+}
+
 void InvokeInst::addAttribute(unsigned i, Attribute::AttrKind attr) {
   AttributeSet PAL = getAttributes();
   PAL = PAL.addAttribute(getContext(), i, attr);
   setAttributes(PAL);
 }
 
 void InvokeInst::removeAttribute(unsigned i, Attribute attr) {
   AttributeSet PAL = getAttributes();