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

[Philip Reames via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

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.


================
Comment at: include/llvm/IR/InstrTypes.h:1201
@@ -1199,6 +1200,3 @@
     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]);
   }
----------------
This appears to be a NFCI refactoring.  Separate the submit without further review.

================
Comment at: include/llvm/IR/InstrTypes.h:1234
@@ -1213,1 +1233,3 @@
+  bool isCapturingOperandBundleUse(const Use &U) const {
+    return isOperandBundleUse(U);
   }
----------------
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.

================
Comment at: include/llvm/IR/Instructions.h:1747
@@ +1746,3 @@
+            (hasClobberingOperandBundles() &&
+             (A == Attribute::ReadOnly || A == Attribute::ReadNone)));
+  }
----------------
The second part of this check should be redundant.  

================
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;
----------------
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.

================
Comment at: lib/Analysis/CaptureTracking.cpp:244
@@ +243,3 @@
+
+      if (CS.isCapturingOperandBundleUse(*U)) {
+        if (Tracker->captured(U))
----------------
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.  

================
Comment at: lib/IR/Instructions.cpp:567
@@ +566,3 @@
+
+  if ((hasReadingOperandBundles() && A == Attribute::ReadNone) ||
+      (hasClobberingOperandBundles() &&
----------------
Duplicate code with above?

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:131
@@ -129,1 +130,3 @@
+    // mayWriteToMemory / mayReadFromMemory based handling kick in.
+    if (CS && !CS.hasOperandBundles()) {
       // Ignore calls to functions in the same SCC.
----------------
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?

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:437
@@ -432,3 +436,3 @@
 
-      if (I->getType()->isVoidTy())
+      if (I->getType()->isVoidTy() && !CS.isCapturingOperandBundleUse(*U))
         Captures = false;
----------------
See previous comment about future bundle author.


http://reviews.llvm.org/D13082