[PATCH] D55018: [CodeExtractor] Split PHI nodes with incoming values from outlined region (PR39433)

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

@kachkov98 thank you for helping to solve this bug. I agree with @vsk 's review comments and I'm most concerned about skipping PHIs with only one predecessor. I think it would be good to add code coverage testing in `Utils/CodeExtractorTest.cpp` explicitly testing your assumptions of the new code in `severSplitPHINodesOfExits()`



================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:619
+    for (PHINode &PN : ExitBB->phis()) {
+      // find all incoming blocks from region
+      SmallVector<unsigned, 2> IncomingVals;
----------------
vsk wrote:
> Nit: please capitalize sentences in comments. Also, maybe "incoming values from the outlining region" would be clearer?
+1

Also please end sentences with a period `.`


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:625
+
+      // do not process PHI if there is one (or fewer) predecessor from region
+      if (IncomingVals.size() <= 1)
----------------
vsk wrote:
> Why should PHIs with exactly one predecessor from the outlining region be skipped? What if there's a second exit block with another, different predecessor from the outlining region?
> 
> ```
>    Out1   ->  Out2
>      \          \ 
>       Exit1      Exit2
> ```
Agreed, I think this needs a little more thought. At the very least it needs a comment justifying why these PHIs should not be processed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55018