[PATCH] D17739: [attrs] Handle convergent CallSites.

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Very nice. Two small nits below, feel free to land with those addressed.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:910-919
@@ -909,37 +909,12 @@
 static bool removeConvergentAttrs(const SCCNodeSet &SCCNodes) {
-  // Determines whether a function can be made non-convergent, ignoring all
-  // other functions in SCC.  (A function can *actually* be made non-convergent
-  // only if all functions in its SCC can be made convergent.)
-  auto CanRemoveConvergent = [&](Function *F) {
-    if (!F->isConvergent())
-      return true;
-
-    // Can't remove convergent from declarations.
-    if (F->isDeclaration())
-      return false;
-
-    for (Instruction &I : instructions(*F))
-      if (auto CS = CallSite(&I)) {
-        // Can't remove convergent if any of F's callees -- ignoring functions
-        // in the SCC itself -- are convergent. This needs to consider both
-        // function calls and intrinsic calls. We also assume indirect calls
-        // might call a convergent function.
-        // FIXME: We should revisit this when we put convergent onto calls
-        // instead of functions so that indirect calls which should be
-        // convergent are required to be marked as such.
-        Function *Callee = CS.getCalledFunction();
-        if (!Callee || (SCCNodes.count(Callee) == 0 && Callee->isConvergent()))
-          return false;
-      }
-
-    return true;
-  };
-
-  // We can remove the convergent attr from functions in the SCC if they all
-  // can be made non-convergent (because they call only non-convergent
-  // functions, other than each other).
-  if (!llvm::all_of(SCCNodes, CanRemoveConvergent))
+  // No point checking if none of SCCNodes is convergent.
+  if (!llvm::any_of(SCCNodes, [](Function *F) { return F->isConvergent(); }))
     return false;
 
-  // If we got here, all of the SCC's callees are non-convergent. Therefore all
-  // of the SCC's functions can be marked as non-convergent.
+  // Can't remove convergent from function declarations.
+  if (llvm::any_of(SCCNodes, [](Function *F) { return F->isDeclaration(); }))
+    return false;
+
+  // Can't remove convergent if any of our functions has a convergent call to a
+  // function not in the SCC.
+  for (Function *F : SCCNodes)
----------------
Perhaps its just me, but I would find this easier to read as a single loop over the functions in the SCC, and three conditions on which we return false (with the exact comments you currently have, those are awesome).

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:923
@@ +922,3 @@
+      CallSite CS(&I);
+      // Bail if is CS a convergent call to a function not in the SCC.
+      if (CS && CS.isConvergent() &&
----------------
"if is CS a" -> "if CS is a"


http://reviews.llvm.org/D17739