[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?

265

Stale comment?

374

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.

387

Please just reuse the two other functions.

387

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 ↗

(On Diff #39149)

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

1614 ↗

(On Diff #39149)

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

353

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.

265

Why stale?

lib/IR/Instructions.cpp

353

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 ↗

(On Diff #39149)

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.

265

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

3500 ↗

(On Diff #39161)

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()); }
 
+  /// param_begin/param_end - Return iterators iterating over the actual
+  /// argument list and bundle operands (i.e. every operand to the instruction
+  /// other than the callee and the two successor blocks for invokes).
+
+  IterTy param_begin() const {
+    assert(getInstruction() && "Not a call or invoke instruction!");
+    // Skip non-arguments
+    return (*this)->op_begin();
+  }
+  IterTy param_end() const { return (*this)->op_end() - (isCall() ? 1 : 3); }
+  iterator_range<IterTy> params() const {
+    return iterator_range<IterTy>(param_begin(), param_end());
+  }
+  bool param_empty() const { return param_end() == param_begin(); }
+  unsigned param_size() const { return unsigned(param_end() - param_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 {
     return isCall() && cast<CallInst>(getInstruction())->isMustTailCall();
   }
 
#define CALLSITE_DELEGATE_SETTER(METHOD) \
   }
 
   /// \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 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 {
     CALLSITE_DELEGATE_GETTER(getDereferenceableBytes(i));
   }
#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 argument is passed by value.
   bool isByValArgument(unsigned ArgNo) const {
     return paramHasAttr(ArgNo + 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 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 if there are is an inalloca argument.  Only the last
   /// 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);
   }

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;

lib/IR/Instructions.cpp

void CallInst::addDereferenceableOrNullAttr(unsigned i, uint64_t Bytes) {
   AttributeSet PAL = getAttributes();
   PAL = PAL.addDereferenceableOrNullAttr(getContext(), i, Bytes);
   setAttributes(PAL);
 }
 
 bool CallInst::paramHasAttr(unsigned i, Attribute::AttrKind A) const {
   assert(i != AttributeSet::FunctionIndex && "Use hasFnAttr instead!");
 
+  if (hasOperandBundles() && i >= (getBundleOperandsStartIndex() + 1))
+    return getOperandBundleForOperand(i - 1).operandsHaveAttr(A);
+
   if (AttributeList.hasAttribute(i, A))
     return true;
   if (const Function *F = getCalledFunction())
     return F->getAttributes().hasAttribute(i, A);
   return false;
 }
 
 /// 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,
                                  BasicBlock *InsertAtEnd, Type *IntPtrTy,
                                  Type *AllocTy, Value *AllocSize, 
                                  Value *ArraySize, Function *MallocF,
                                  const Twine &Name) {
   assert(((!InsertBefore && InsertAtEnd) || (InsertBefore && !InsertAtEnd)) &&
          "createMalloc needs either InsertBefore or InsertAtEnd");
 
   // malloc(type) becomes: 
bool InvokeInst::hasFnAttrImpl(Attribute::AttrKind A) const {
   if (const Function *F = getCalledFunction())
     return F->getAttributes().hasAttribute(AttributeSet::FunctionIndex, A);
   return false;
 }
 
 bool InvokeInst::paramHasAttr(unsigned i, Attribute::AttrKind A) const {
   assert(i != AttributeSet::FunctionIndex && "Use hasFnAttr instead!");
 
+  if (hasOperandBundles() && i >= (getBundleOperandsStartIndex() + 1))
+    return getOperandBundleForOperand(i - 1).operandsHaveAttr(A);
+
   if (AttributeList.hasAttribute(i, A))
     return true;
   if (const Function *F = getCalledFunction())
     return F->getAttributes().hasAttribute(i, A);
   return false;
 }
 
 void InvokeInst::addAttribute(unsigned i, Attribute::AttrKind attr) {