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

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
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)
----------------
chandlerc wrote:
> 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).
Hm, it's slightly more complicated than that, because the first 'if' is a !any_of.  But I went ahead and made the change, because I think it's justified on the grounds of not wanting to iterate over a non-convergent function's instructions if it happens to be in an SCC with a convergent function.


Repository:
  rL LLVM

http://reviews.llvm.org/D17739