[PATCH] D75440: [Coroutines] Optimized coroutine elision based on reachability

[Brian Gesiak via Phabricator via llvm-commits]

modocache added a comment.

> yep, I have tried the two samples, same behavior as clang6.

Phenomenal!! Thanks for checking. Please feel free to address the nit-pick comments only if you wish. I'll wait to accept until I better understand the rationale for checking the number of cases in a suspend switch statement, if that's alright.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroElide.cpp:33
   SmallVector<CoroFreeInst *, 1> CoroFrees;
-  CoroSuspendInst *CoroFinalSuspend;
+  SmallPtrSet<SwitchInst *, 4> CoroSusSwitchs;
 
----------------
A nit-pick, but: it's spelled "switches" with an "e". So I would rename this `CoroSuspendSwitches`, or if that's too long of a variable name for you, `CoroSwitches`. Also, this could be marked `const` to signal these won't be "lowered" or transformed in any way: `SmallPtrSet<const SwitchInst *, 4>`.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroElide.cpp:147
 
+bool Lowerer::hasEscapePath(CoroBeginInst *CB,
+                            const SmallPtrSetImpl<BasicBlock *> &TIs) const {
----------------
Another nit-pick: this can be `const CoroBeginInst *` (with a few modifications below, like `SmallVector<const BasicBlock *, 32> Worklist;`). I think it'd be a small improvement because it'd make clear that this function just analyzes paths through the IR, it doesn't modify it.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroElide.cpp:172
+    // Conservatively say that there is potentially a path.
+    if (!--Limit)
+      return true;
----------------
Yet another nit-pick, sorry: maybe it's just me but I would find this much clearer with `--Limit;` as its own statement on its own line. Just my opinion, though, please ignore it if you disagree.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroElide.cpp:251
+        SwitchInst *SWI = cast<SwitchInst>(CSI->use_begin()->getUser());
+        // Only collect switchInsts that donot changed
+        if (SWI->getNumCases() == 2)
----------------
Typo: "donot" should be "do not" -- but also, this is the first instance in which these coroutine passes check for the number of cases in a switch statement. Could you expand on this comment? I'm afraid I don't follow why coro.suspend with switches between resume/cleanup should be part of the path analysis, but others should not.


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

https://reviews.llvm.org/D75440