llvm dev - Oct 2015 - 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

On Thu, Oct 22, 2015 at 4:46 PM Sanjoy Das via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20151023/aa77b00d/attachment.html>