A design question about operand bundles

Currently I'm trying to "port" over code like this to do the right
thing for calls with operand bundles:

      CallSite::arg_iterator B = CS.arg_begin(), E = CS.arg_end();
      for (CallSite::arg_iterator A = B; A != E; ++A, ++AI) {
        if (A->get() == V) {
          if (AI == AE) {
            assert(F->isVarArg() &&
                   "More params than args in non-varargs call.");
            return Attribute::None;
          }
          Captures &= !CS.doesNotCapture(A - B);
          if (SCCNodes.count(&*AI))
            continue;
          if (!CS.onlyReadsMemory() && !CS.onlyReadsMemory(A - B))
            return Attribute::None;
          if (!CS.doesNotAccessMemory(A - B))
            IsRead = true;
        }
      }

The above snippet is from FunctionAttrs.cpp, but there likely are
other similar examples.

The problem here that the piece of code above assumes that all
interesting inputs to a call / invoke are arguments, and if it
does not find V in the CallSite's argument list then the CallSite does
not read, write or capture it. This is no longer true with operand
bundles.

I'm thinking of addressing this by adding an `input_begin()` /
`input_end()` pair of accessors to `CallSite`. The
`input_begin()` .. `input_end()` range would include all of the
arguments in the `CallSite` *and* all of the operand bundle inputs.
Then I'd generalize accessors on `CallSite` like `doesNotAccessMemory`
and `onlyReadsMemory` to look up either the relevant
`OperandBundleUse` or the `AttributeSet` as appropriate. Does this
sound reasonable overall?

A related design point is the depth at which this logic to choose
between the attribute list and operand bundles should live at. My
intent was to initially keep this as a convenience wrapper in
CallSite, but I can just as easily see this living directly on
`CallInst` and `InvokeInst`. Perhaps `AttributeSet` needs to directly
support operand bundles? That may come in handy when later we add
`readonly` `"deopt"` operand bundles.

Another option is to guard loops like the above with
`!CS.isOperandBundleUse(*U)`. This is what I tried in
http://reviews.llvm.org/D13082 and Philip (rightly) pointed out that
having to do this really shows that something is lacking in the
internal APIs.

Thanks!
-- Sanjoy

I think the whole point of getting operand bundles fully into the IR is, well, to put them fully into the IR.

I like your suggestion of iterators / ranges.

I would probably sink them as far down into the IR as it makes sense. I’m pretty sure this means CallInst and InvokeInst. I’m not sure whether it would make sense to teach AttributeSet about it – if you look into it and it makes engineering sense one way or the other, go that way, mail out the patch. =D