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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:635
+                                   ExitBB->getParent(), ExitBB);
+        for (pred_iterator I = pred_begin(ExitBB), E = pred_end(ExitBB);
+             I != E;) {
----------------
kachkov98 wrote:
> vsk wrote:
> > Why not "for (BasicBlock &PredBB : predecessors(ExitBB))"?
> The reason is that iterators become invalidated when terminator instuction in predecessor is updated
Actually, as-written, there is an iterator invalidation bug here. This fixes it:
```
SmallVector<BasicBlock *, 4> Preds(pred_begin(ExitBB), pred_end(ExitBB));
for (BasicBlock *PredBB : Preds) { ... }
```

This handles the case where one (or more) of the predecessor blocks is terminated by a switch, which can have multiple uses of ExitBB. The current version of the code walks the wrong user list when this happens.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:650
+      for (unsigned i : IncomingVals)
+        NewPN->addIncoming(PN.getIncomingValue(i), PN.getIncomingBlock(i));
+      for (unsigned i : reverse(IncomingVals))
----------------
kachkov98 wrote:
> vsk wrote:
> > When you replace all uses of ExitBB with NewBB for every terminator in a predecessor of ExitBB, every new PHI inserted into NewBB must have *some* incoming value (just undef?) from *each* predecessor of NewBB. Otherwise, the verifier complains (taken from a stage2 build with hot/cold splitting enabled):
> > ```
> > PHINode should have one entry for each predecessor of its parent basic block!
> >   %conv97.lcssa106.ce = phi i64 [ 1, %if.then31.1 ], [ 1, %if.then31.1 ], [ 2, %if.then31.2 ], [ 2, %if.then31.2 ]
> > fatal error: error in backend: verification of newFunction failed!
> > ```
> This error message is quite ambigious) I think it means here that PHI shouldn't have duplicated incoming blocks (for example, 2 %if.then31.1 blocks are not correct - how to choose value in that case?). Nevertheless, severSplitPHINodesOfExits just copies these incoming blocks from original PHI, and it means that this PHI was incorrect *before* the extraction. I can confirm that already have this failure while compiling test-suite with HotColdSplitting and PGO, and checking that function is consistent before any extraction resolved it. This check can be done in CodeExtractor (lines 1191-1195), but root cause is that some pass creates this PHI before.
The verifier allows duplicate incoming blocks (as long as they provide a unique incoming value). This isn't a bug in another pass -- I hit the same verification failure with your most recent patch, which verifies oldFunction before doing any work.

The suggested iterator invalidation fix in my comment above addresses the problem.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:1192
+  // It is critical for extractor that PHIs have one entry for each predecessor.
+  if (verifyFunction(*oldFunction)) {
+    LLVM_DEBUG(dbgs() << "verifyFunction failed, extraction not performed!\n");
----------------
This should be wrapped in LLVM_DEBUG(), as we don't want to pay the compile-time cost for this in Release builds

 There are build bots which test llvm using -verify-each (inserting the verifier after each pass), so (at least theoretically) we should safely be able to assume the input IR is correct.


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

https://reviews.llvm.org/D55018