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

[Philip Reames via llvm-commits]

reames added inline comments.

================
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;
----------------
sanjoy wrote:
> 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).
I would *strongly* encourage you to improve existing interfaces where required to hide the details of operand bundles.  Every pass change should have a very high bar because it greatly increases the odds of future errors.

I would also encourage you *not* to keep patches out of tree.  You should write test cases for the most general possible bundle and check in each test.  I agree that getting out of tree deopt bundle working is useful, but I feel the advantage of being able to work incrementally and collaborate with other ongoing work substantially outweighs the benefit of prototyping everything.  


http://reviews.llvm.org/D13082