[FunctionAttrs] Conservatively handle operand bundles.

[sanjoy]

Summary

This teaches the FunctionAttrs pass to handle operand bundles
conservatively. All calls / invokes with operand bundles are
currently considered to be reading from and writing to the entire
heap.

I've also added a test case PruneEH to show that a call with operand
bundles inherits its callee's behavior with respect to unwinding -- if
the callee is nounwind, the call is nounwind too.

Depends on D12457.

[sanjoy]

• rebase over recent to FunctionAttrs.cpp

[sanjoy]

ping!

[reames]

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.

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.

include/llvm/IR/InstrTypes.h

1201

This appears to be a NFCI refactoring. Separate the submit without further review.

1234

I'd add a comment here about how individual bundles might refine this later.

Also, not sure this is the API we want. Would it be better to get the containing bundle, then ask *it* whether the use is capturing? Same for each of the predicated below.

include/llvm/IR/Instructions.h

1747

The second part of this check should be redundant.

1751

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.

lib/Analysis/CaptureTracking.cpp

244

I don't think this line is needed currently since an unknown bundle will fail the following check. Please let whoever adds a non-capturing bundle worry about this.

lib/IR/Instructions.cpp

567

Duplicate code with above?

lib/Transforms/IPO/FunctionAttrs.cpp

131

This looks suspicious. We should be able to take the CS path. If we can't, I suspect you're missing semantically required hooks in AA, specifically, probably getModRefBehavior? Unless you're claiming that bundles prevent us from using the call within SCC reasoning?

437

See previous comment about future bundle author.

[sanjoy]

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

In 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.

include/llvm/IR/Instructions.h

1751

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).

[reames]

include/llvm/IR/Instructions.h

1751

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.

[sanjoy]

This has been superseded by D13961 and D13962.

include/llvm/IR/CallSite.h

#define CALLSITE_DELEGATE_SETTER(METHOD) \
   int getNumTotalBundleOperands() const {
     CALLSITE_DELEGATE_GETTER(getNumTotalBundleOperands());
   }
 
   OperandBundleUse getOperandBundle(unsigned Index) const {
     CALLSITE_DELEGATE_GETTER(getOperandBundle(Index));
   }
 
+  bool isOperandBundleUse(const Use &U) const {
+    CALLSITE_DELEGATE_GETTER(isOperandBundleUse(U));
+  }
+
+  bool isCapturingOperandBundleUse(const Use &U) const {
+    CALLSITE_DELEGATE_GETTER(isCapturingOperandBundleUse(U));
+  }
+
 #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);
   }
 

include/llvm/IR/InstrTypes.h

 // i*.h files...
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_INSTRTYPES_H
 #define LLVM_IR_INSTRTYPES_H
 
 #include "llvm/ADT/Twine.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/OperandTraits.h"
 
 namespace llvm {
 
 class LLVMContext;
 
unsigned getNumTotalBundleOperands() const {
     assert(Begin <= Back && "hasOperandBundles() returned true!");
 
     return Back->End - Begin->Begin;
   }
 
   /// \brief Return the operand bundle at a specific index.
   OperandBundleUse getOperandBundle(unsigned Index) const {
     assert(Index < getNumOperandBundles() && "Index out of bounds!");
-    auto *BOI = bundle_op_info_begin() + Index;
-    auto op_begin = static_cast<const InstrTy *>(this)->op_begin();
-    ArrayRef<Use> Inputs(op_begin + BOI->Begin, op_begin + BOI->End);
-    return OperandBundleUse(BOI->Tag->getKey(), Inputs);
+    return bundleOpInfoToOpBundleUse(bundle_op_info_begin()[Index]);
   }
 
   /// \brief Return the operand bundle at a specific index.
   OperandBundleUse getOperandBundle(unsigned Index) {
     assert(Index < getNumOperandBundles() && "Index out of bounds!");
-    auto *BOI = bundle_op_info_begin() + Index;
-    auto op_begin = static_cast<InstrTy *>(this)->op_begin();
-    ArrayRef<Use> Inputs(op_begin + BOI->Begin, op_begin + BOI->End);
-    return OperandBundleUse(BOI->Tag->getKey(), Inputs);
+    return bundleOpInfoToOpBundleUse(bundle_op_info_begin()[Index]);
+  }
+
+  /// \brief Return the operand bundle containing U, and None if U is not
+  /// contained in an operand bundle.
+  Optional<OperandBundleUse> getContainingOperandBundle(const Use &U) const {
+    if (!hasOperandBundles())
+      return None;
+
+    unsigned UseIndex = &U - static_cast<const InstrTy *>(this)->op_begin();
+    assert(UseIndex < static_cast<const InstrTy *>(this)->getNumOperands() &&
+           "U not a Use for this User!");
+
+    for (auto &BOI : bundle_op_infos())
+      if (UseIndex <= BOI.Begin && UseIndex < BOI.End)
+        return bundleOpInfoToOpBundleUse(BOI);
+
+    return None;
+  }
+
+  /// \brief Return true if the use U is an operand bundle use.
+  bool isOperandBundleUse(const Use &U) const {
+    return getContainingOperandBundle(U).hasValue();
+  }
+
+  /// \brief Return true if U captured by via an operand bundle.
+  bool isCapturingOperandBundleUse(const Use &U) const {
+    return isOperandBundleUse(U);
   }
 
+  /// \brief Return true if this operand bundle user has operand bundles that
+  /// may read from the heap.
+  bool hasReadingOperandBundles() const { return hasOperandBundles(); }
+
+  /// \brief Return true if this operand bundle user has operand bundles that
+  /// may write to the heap.
+  bool hasClobberingOperandBundles() const { return hasOperandBundles(); }
+
 protected:
   /// \brief Used to keep track of an operand bundle.  See the main comment on
   /// OperandBundleUser above.
   struct BundleOpInfo {
     /// \brief The operand bundle tag, interned by
     /// LLVMContextImpl::getOrInsertBundleTag.
     StringMapEntry<uint32_t> *Tag;
 
     /// \brief The index in the Use& vector where operands for this operand
     /// bundle starts.
     uint32_t Begin;
 
     /// \brief The index in the Use& vector where operands for this operand
     /// bundle ends.
     uint32_t End;
   };
 
+  /// \brief Helper function create an OperandBundleUse out of an BundleOpInfo.
+  OperandBundleUse bundleOpInfoToOpBundleUse(const BundleOpInfo &BOI) const {
+    auto op_begin = static_cast<const InstrTy *>(this)->op_begin();
+    ArrayRef<Use> Inputs(op_begin + BOI.Begin, op_begin + BOI.End);
+    return OperandBundleUse(BOI.Tag->getKey(), Inputs);
+  }
+
   typedef BundleOpInfo *bundle_op_iterator;
   typedef const BundleOpInfo *const_bundle_op_iterator;
 
   /// \brief Return the start of the list of BundleOpInfo instances associated
   /// with this OperandBundleUser.
   bundle_op_iterator bundle_op_info_begin() {
     if (!static_cast<InstrTy *>(this)->hasDescriptor())
       return nullptr;

include/llvm/IR/Instructions.h

public:
   static inline bool classof(const Instruction *I) {
     return I->getOpcode() == Instruction::Call;
   }
   static inline bool classof(const Value *V) {
     return isa<Instruction>(V) && classof(cast<Instruction>(V));
   }
 
 private:
+  bool isFnAttrDisallowedByOpBundle(StringRef S) const { return false; }
+
+  bool isFnAttrDisallowedByOpBundle(Attribute::AttrKind A) const {
+    return ((hasReadingOperandBundles() && A == Attribute::ReadNone) ||
+            (hasClobberingOperandBundles() &&
+             (A == Attribute::ReadOnly || A == Attribute::ReadNone)));
+  }
+
   template <typename AttrKind> bool hasFnAttrImpl(AttrKind A) const {
     if (AttributeList.hasAttribute(AttributeSet::FunctionIndex, A))
       return true;
+
+    if (isFnAttrDisallowedByOpBundle(A))
+      return false;
+
     if (const Function *F = getCalledFunction())
       return F->getAttributes().hasAttribute(AttributeSet::FunctionIndex, A);
     return false;
   }
 
   // Shadow Instruction::setInstructionSubclassData with a private forwarding
   // method so that subclasses cannot accidentally use it.
   void setInstructionSubclassData(unsigned short D) {

lib/Analysis/CaptureTracking.cpp

while (!Worklist.empty()) {
     const Use *U = Worklist.pop_back_val();
     Instruction *I = cast<Instruction>(U->getUser());
     V = U->get();
 
     switch (I->getOpcode()) {
     case Instruction::Call:
     case Instruction::Invoke: {
       CallSite CS(I);
+
+      if (CS.isCapturingOperandBundleUse(*U)) {
+        if (Tracker->captured(U))
+          return;
+        break;
+      }
+
       // Not captured if the callee is readonly, doesn't return a copy through
       // its return value and doesn't unwind (a readonly function can leak bits
       // by throwing an exception or not depending on the input value).
       if (CS.onlyReadsMemory() && CS.doesNotThrow() && I->getType()->isVoidTy())
         break;
 
       // Not captured if only passed via 'nocapture' arguments.  Note that
       // calling a function pointer does not in itself cause the pointer to

lib/IR/Instructions.cpp

 }
 void InvokeInst::setSuccessorV(unsigned idx, BasicBlock *B) {
   return setSuccessor(idx, B);
 }
 
 bool InvokeInst::hasFnAttrImpl(Attribute::AttrKind A) const {
   if (AttributeList.hasAttribute(AttributeSet::FunctionIndex, A))
     return true;
+
+  if ((hasReadingOperandBundles() && A == Attribute::ReadNone) ||
+      (hasClobberingOperandBundles() &&
+       (A == Attribute::ReadOnly || A == Attribute::ReadNone)))
+    return false;
+
   if (const Function *F = getCalledFunction())
     return F->getAttributes().hasAttribute(AttributeSet::FunctionIndex, A);
   return false;
 }
 
 bool InvokeInst::paramHasAttr(unsigned i, Attribute::AttrKind A) const {
   if (AttributeList.hasAttribute(i, A))
     return true;

lib/Transforms/IPO/FunctionAttrs.cpp

checkFunctionMemoryAccess(Function &F, AAResults &AAR,
   // Scan the function body for instructions that may read or write memory.
   bool ReadsMemory = false;
   for (inst_iterator II = inst_begin(F), E = inst_end(F); II != E; ++II) {
     Instruction *I = &*II;
 
     // Some instructions can be ignored even if they read or write memory.
     // Detect these now, skipping to the next instruction if one is found.
     CallSite CS(cast<Value>(I));
-    if (CS) {
+
+    // If the call or invoke has operand bundles, we let the generic
+    // mayWriteToMemory / mayReadFromMemory based handling kick in.
+    if (CS && !CS.hasOperandBundles()) {
       // Ignore calls to functions in the same SCC.
       if (CS.getCalledFunction() && SCCNodes.count(CS.getCalledFunction()))
         continue;
       FunctionModRefBehavior MRB = AAR.getModRefBehavior(CS);
       // If the call doesn't access arbitrary memory, we may be able to
       // figure out something.
       if (AliasAnalysis::onlyAccessesArgPointees(MRB)) {
         // If the call does access argument pointees, check each argument.
case Instruction::AddrSpaceCast:
       for (Use &UU : I->uses())
         if (Visited.insert(&UU).second)
           Worklist.push_back(&UU);
       break;
 
     case Instruction::Call:
     case Instruction::Invoke: {
       bool Captures = true;
+      CallSite CS(I);
 
-      if (I->getType()->isVoidTy())
+      if (I->getType()->isVoidTy() && !CS.isCapturingOperandBundleUse(*U))
         Captures = false;
 
       auto AddUsersToWorklistIfCapturing = [&] {
         if (Captures)
           for (Use &UU : I->uses())
             if (Visited.insert(&UU).second)
               Worklist.push_back(&UU);
       };
 
-      CallSite CS(I);
       if (CS.doesNotAccessMemory()) {
         AddUsersToWorklistIfCapturing();
         continue;
       }
 
       Function *F = CS.getCalledFunction();
-      if (!F) {
+      if (!F || CS.isOperandBundleUse(*U)) {
         if (CS.onlyReadsMemory()) {
           IsRead = true;
           AddUsersToWorklistIfCapturing();
           continue;
         }
         return Attribute::None;
       }
 

test/Transforms/FunctionAttrs/operand-bundles.ll

+; RUN: opt -functionattrs -S < %s | FileCheck %s
+
+declare void @readnone_callee() readnone
+
+; %ptr should not be marked readnone or nocapture.
+
+; CHECK: define void @f(i32* %ptr) {
+define void @f(i32* %ptr) {
+ entry:
+  call void @readnone_callee() [ "foo"(i32* %ptr) ]
+  ret void
+}

test/Transforms/PruneEH/operand-bundles.ll

+; RUN: opt -prune-eh -S < %s | FileCheck %s
+
+declare void @readnone_callee() nounwind
+
+; CHECK: define void @f(i32* %ptr) #0 {
+define void @f(i32* %ptr) {
+ entry:
+  call void @readnone_callee() [ "foo"(i32* %ptr) ]
+  ret void
+}
+
+; CHECK: attributes #0 = { nounwind }