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

Mon Dec 3 12:46:20 PST 2018

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

Thank you, LGTM.

I tested this patch by:

1. Building LNT+externals with hot/cold splitting enabled. I forced outlining to occur whenever a block has more than 1 predecessor, so long as it wouldn't result in the entire function being outlined. This resulted in 48,441 cold functions being extracted/outlined. All output validation tests still passed.
2. Running check-llvm in a stage2 build with hot/cold splitting enabled in the same way described above.

================
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:
> > vsk wrote:
> > > 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.
> > By the way, the in-tree version of the hot/cold splitting pass has several known issues. If you're interested in that pass, I have some patches up to address them: https://reviews.llvm.org/D53887, https://reviews.llvm.org/D54189, https://reviews.llvm.org/D54244. I'd certainly appreciate your feedback!
> Currently I'm facing the problem that hotcoldsplitting adds to ColdRegion one basic block 2 times (first time when visiting predecessors of SinkBB, second time when visitting successors) - it causes CodeExtractor check fail. Not sure that these patches don't solve the problem, but if it is interesting, I can provide test case for this (what is the best way to do it?)
D54189 fixes that bug, but I'm not sure whether it still applies cleanly. I'll update it soon.

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

https://reviews.llvm.org/D55018