[PATCH] D39607: [PartialInliner] Inline vararg functions that forward varargs.

[Davide Italiano via Phabricator via llvm-commits]

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

Really nice :) Some comments inline to begin with.



================
Comment at: include/llvm/Transforms/Utils/CodeExtractor.h:92
     /// Blocks containing EHPads, allocas, invokes, or vastarts are not valid.
-    static bool isBlockValidForExtraction(const BasicBlock &BB);
+    static bool isBlockValidForExtraction(const BasicBlock &BB,
+                                          bool AllowVarArgs);
----------------
This should presumably have a comment explaining what the additional bool controls.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:177-178
     : DT(&DT), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
-      BPI(BPI), Blocks(buildExtractionBlockSet(L.getBlocks(), &DT)) {}
+      BPI(BPI), Blocks(buildExtractionBlockSet(L.getBlocks(), &DT, false)) {}
 
 /// definedInRegion - Return true if the specified value is defined in the
----------------
Mind to add a comment next to the `false` with the member variable name :) ?


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1829
          ++BB) {
-      for (Instruction &I : *BB) {
+      for (auto II = BB->begin(); II != BB->end();) {
+        Instruction &I = *II++;
----------------
Are you switching this because of iterator invalidation issues?
In this case, maybe it would be cleaner to collect the set of instructions to be removed in a set, and then remove them at the end of this loop?


https://reviews.llvm.org/D39607