[PATCH] D33618: [PartialInlining] Reduce function outlining overhead

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

This is a really important optimization, IMHO (and for my usecase). Thanks for working on this. 
Some comments inline.



================
Comment at: include/llvm/Transforms/Utils/CodeExtractor.h:99-108
     /// \brief Compute the set of input values and output values for the code.
     ///
     /// These can be used either when performing the extraction or to evaluate
     /// the expected size of a call to the extracted function. Note that this
     /// work cannot be cached between the two as once we decide to extract
     /// a code sequence, that sequence is modified, including changing these
     /// sets, before extraction occurs. These modifications won't have any
----------------
Can you update this comment to reflect what's the Allocas argument here?
Also, probably this should be names `SinkCandidates` or something?


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:145
 
-void CodeExtractor::findInputsOutputs(ValueSet &Inputs,
-                                      ValueSet &Outputs) const {
+void CodeExtractor::findInputsOutputs(ValueSet &Inputs, ValueSet &Outputs,
+                                      ValueSet &SinkCands) const {
----------------
Now that I look at this more closely, `findInputsOutputs` is a slightly inaccurate description after you change. Maybe we can consider splitting this in a separate function, as this one is growing quite a bit?


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:178-188
+      bool FoundUnknownUse = false;
+      if (AI != LocalWithBitcast)
+        for (User *U : AI->users()) {
+          if (U != LocalWithBitcast && !definedInRegion(Blocks, U)) {
+            FoundUnknownUse = true;
+            break;
+          }
----------------
I think this can be expressed much more concisely using `std/llvm::any_of`.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:200-202
       for (User::op_iterator OI = II.op_begin(), OE = II.op_end(); OI != OE;
-           ++OI)
-        if (definedInCaller(Blocks, *OI))
-          Inputs.insert(*OI);
+           ++OI) {
+        Value *V = *OI;
----------------
While you're here, I'd see if you can use a `range for` instead (and maybe avoid the extra assignment to value, but I'm not sure if it's possible).


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:817
+  for (auto *II : SinkingCands)
+    dyn_cast<Instruction>(II)->moveBefore(*newFuncRoot,
+                                          newFuncRoot->getFirstInsertionPt());
----------------
I'm not entirely sure I like this bit.
`dyn_cast<>` could return `nullptr`, resulting in trouble when deferencing. If you're always guaranteed to get a function back, I'd use `cast<>`, (or, alternatively use `if (dyn_cast<>)`


https://reviews.llvm.org/D33618