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

(On Diff #39149)

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

173 ↗

(On Diff #39149)

Why have this assert here and no where else?

268 ↗

(On Diff #39149)

Stale comment?

381 ↗

(On Diff #39149)

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 ↗

(On Diff #39149)

Please just reuse the two other functions.

397 ↗

(On Diff #39149)

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

include/llvm/IR/InstrTypes.h

1237 ↗

(On Diff #39149)

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

350 ↗

(On Diff #39149)

This would also catch FunctionIndex. Intentional?

[sanjoy]

include/llvm/IR/CallSite.h

173 ↗

(On Diff #39149)

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

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

268 ↗

(On Diff #39149)

Why stale?

lib/IR/Instructions.cpp

350 ↗

(On Diff #39149)

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 ↗

(On Diff #39161)

use std::distance here.

273 ↗

(On Diff #39161)

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 ↗

(On Diff #39161)

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 ↗

(On Diff #39161)

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 ↗

(On Diff #39161)

Ah. Duh. :)

llvm/trunk/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()); }
 
+  /// Type of iterator to use when looping over data operands at this call site
+  /// (see below).
+  typedef IterTy data_operand_iterator;
+
+  /// data_operands_begin/data_operands_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_operands_begin() const {
+    assert(getInstruction() && "Not a call or invoke instruction!");
+    return (*this)->op_begin();
+  }
+  IterTy data_operands_end() const {
+    assert(getInstruction() && "Not a call or invoke instruction!");
+    return (*this)->op_end() - (isCall() ? 1 : 3);
+  }
+  iterator_range<IterTy> data_ops() const {
+    return iterator_range<IterTy>(data_operands_begin(), data_operands_end());
+  }
+  bool data_operands_empty() const {
+    return data_operands_end() == data_operands_begin();
+  }
+  unsigned data_operands_size() const {
+    return std::distance(data_operands_begin(), data_operands_end());
+  }
+
   /// 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(); }
 
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 at index \p i directly or
+  /// indirectly has the attribute \p A.
+  ///
+  /// Normal call or invoke arguments have per operand attributes, as specified
+  /// in the attribute set attached to this instruction, while operand bundle
+  /// operands may have some attributes implied by the type of its containing
+  /// operand bundle.
+  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 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);
+  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;

llvm/trunk/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.
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();
   }

llvm/trunk/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.
+  ///
+  /// The index \p i is interpreted as
+  ///
+  ///  \p i == Attribute::ReturnIndex  -> the return value
+  ///  \p i in [1, arg_size + 1)  -> argument number (\p i - 1)
+  ///  \p i in [arg_size + 1, data_operand_size + 1) -> bundle operand at index
+  ///     (\p i - 1) in the operand list.
+  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.
+  ///
+  /// The index \p i is interpreted as
+  ///
+  ///  \p i == Attribute::ReturnIndex  -> the return value
+  ///  \p i in [1, arg_size + 1)  -> argument number (\p i - 1)
+  ///  \p i in [arg_size + 1, data_operand_size + 1) -> bundle operand at index
+  ///     (\p i - 1) in the operand list.
+  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 {

llvm/trunk/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();