[PATCH] D17013: Add convergent-removing bits to FunctionAttrs pass.

[Justin Lebar via llvm-commits]

jlebar added a comment.

Thank you for the reviews.  I've updated the patch, please have a look when you have a chance.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:936
@@ +935,3 @@
+static bool removeConvergentAttrs(const CallGraphSCC &SCC) {
+  const CallGraphNode *CGN = *SCC.begin();
+  Function *F = CGN->getFunction();
----------------
joker.eph wrote:
> jingyue wrote:
> > `SCC` may contain multiple functions. In that case, you need to consider all of them. 
> That's a good point: any reason not to take the `SCCNodeSet` and iterate over like `addNonNullAttrs` for instance?
> SCC may contain multiple functions. In that case, you need to consider all of them.

Thanks, this was quite broken.

> any reason not to take the SCCNodeSet?

SCCNodeSet is the set of strongly-connected nodes -- aiui a set of mutually-recursive functions, excluding any OptimizeNone or external functions that may be present in the set.

Two problems with using this rather than SCC itself.  First, I think we do want to consider OptimizeNone functions.  At least, if Foo and Bar are convergent and mutually recursive and Bar is OptimizeNone, then Foo should remain convergent.  (The original code didn't handle OptimizeNone correctly either; I've fixed this.)

Second, if we use this, we don't get the actual control-flow graph, which is contained in SCC.  There appears to be nontrivial logic to e.g. devirtualize calls in there, so I don't think we want to replicate all that code when we iterate over the function looking for calls to convergent intrinsics.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:949
@@ +948,3 @@
+  // body looking for those.
+  for (auto &BB : *F)
+    for (auto &I : BB) {
----------------
jingyue wrote:
> You can iterate through the call sites with `CallGraphNode::iterator` (http://llvm.org/docs/doxygen/html/CallGraph_8h_source.html#l00186). 
I may be misunderstanding, but I think that's what we're doing in the loop above when we do llvm::any_of(*CGN) -- we're iterating over CallGraphNode::begin()/end().  The issue is that, as the CallGraphNode is constructed in CGPassManager::RefreshCallGraph, does not contain calls to intrinsic functions.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:954
@@ +953,3 @@
+      Function *Callee = CS.getCalledFunction();
+      if (Callee && Callee->isIntrinsic() && Callee->isConvergent())
+        return false;
----------------
joker.eph wrote:
> `Callee->isIntrinsic()` should be redundant here (I mean `!Callee->isIntrinsic() && Callee->isConvergent()` should be impossible after the `llvm::any_of` loop above right?
You're right, it wasn't necessary.  However, it now is, because now we're ignoring calls to functions in the same SCC.


http://reviews.llvm.org/D17013