[PATCH] D53706: [MultiTailCallElimination]: Pass to eliminate multiple tail calls

[John Brawn via Phabricator via llvm-commits]

john.brawn added inline comments.


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:547-553
+  if ((SizeLevel == 0) && (OptLevel > 2)) {
+    MPM.add(createSROAPass());
+    MPM.add(createCFGSimplificationPass());     // Merge & remove BBs
+    MPM.add(createMultiTailCallEliminationPass());
+    MPM.add(createVerifierPass());
+    MPM.add(createAggressiveDCEPass());         // Delete dead instructions
+  }
----------------
I would think that just before the TailCallEliminationPass would be the place to run this pass, and from experimentation doing it there we don't need to add any other passes around it.


================
Comment at: lib/Transforms/Scalar/MultiTailCallElimination.cpp:405-407
+    RecursiveCall(const RecursiveCall &O) : Index(O.Index), F(O.F), H(O.H), CI(O.CI), Is() {
+      llvm_unreachable("RecursiveCall cannot be copied");
+    }
----------------
It would be simpler to do RecursiveCall(const RecursiveCall &O) = delete.


================
Comment at: lib/Transforms/Scalar/MultiTailCallElimination.cpp:1507-1513
+      if (&*BBI == TI)
+        (&*BBI == E) {
+        StaticInst.push_back(&*BBI);
+        // Advance the current iteration interval to next candidate.
+        ++CandI;
+        continue;
+      }
----------------
john.brawn wrote:
> This doesn't even compile.  If I turn the condition into ((&*BBI == E) || (&*BBI == TI)) or one of those two on their own I get an assertion failure on ++CandI because CandI is Candidates.end().
I looks like maybe the while(true) should be while(CandI != Candidates.end()). With that the tests pass at least.


================
Comment at: lib/Transforms/Scalar/MultiTailCallElimination.cpp:1626-1627
+    }
+    else {
+      if (OtherCandidatesNumOf == 1) {
+        Marker.reset(new TrueMarker(C, OtherCandidatesNumOf));
----------------
Should be 'else if' instead of nested 'if'. Also the curly braces are unnecessary here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53706/new/

https://reviews.llvm.org/D53706