[PATCH] D13082: [FunctionAttrs] Conservatively handle operand bundles.

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

(I'll reply to your inline review comments individually in some time, this is reply to the meta questions you raised)

In http://reviews.llvm.org/D13082#263070, @reames wrote:

> Meta comment:  I really don't feel like FunctionAttrs is the right place to start.  You need to work through a lot of the API questions and there are far simpler passes to work from.  In particular, EarlyCSE would let you write test cases for most of the aliasing properties you're changing.


`FunctionAttrs` seemed more "direct" as that lets me check the
inferred directly.  I will soon start auditing other places in LLVM,
and I check those in first if you prefer.

> Second meta: The fact that you need to change *any* pass should seem really really suspicious.  The most conservative implementation shouldn't need pass specific hooks.


I'll reply to your inline comments individually, but generally the
problematic case (I'll make this clearer in the code) is when you have
things like

  for (Value *A : CS.getArgs()) {
    if (!mayAlias(A, ThingIAmInterestedIn) && A->isReadOnlyArg())
      MayClobberThingIAmInterestedIn = true;
  }
  
  if (!MayClobberThingIAmInterestedIn)
    ...

An operand bundle use will simply not show up in `CS.getArgs`, and as
I write this I realize that I should really be fixing the loop to
iterate over all inputs to the call instruction including operand bundles

- I'll do that restructuring.


================
Comment at: include/llvm/IR/Instructions.h:1751
@@ -1742,3 +1750,3 @@
   template <typename AttrKind> bool hasFnAttrImpl(AttrKind A) const {
     if (AttributeList.hasAttribute(AttributeSet::FunctionIndex, A))
       return true;
----------------
reames wrote:
> You appear to be going for the semantic that a bundle can override a function attribute, but not a call site attribute?  If so, maybe a comment.  This also needs clarified in the LangRef.
TBQH, that issue is not settled in my mind yet.  I was surprised to
see that virtually nothing in LLVM "oversteps" the attributes in
`CallSite` to get the attributes directly from the `Function *` so
that may make implementing this scheme (calls can read/write even if
what they call is `readnone`) easier than it first looked.

My plan here is to keep this patch and patches based off of this out
of tree until I'm reasonably certain I'm not missing something
fundamental (in practice this means getting our internal workloads
successfully keeping deopt state in operand bundles through a significant
portion of pipeline).


http://reviews.llvm.org/D13082