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

[Sergei Kachkov via Phabricator via llvm-commits]

kachkov98 marked 5 inline comments as done.
kachkov98 added inline comments.


================
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)
----------------
brzycki wrote:
> 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.
My intention here was to not create unnecessary output parameters when this single value is constant (if we split it to separate phi, findInputsOutputs() detects that PHI value is used outside the region and creates new argument). If this value is not constant, it will be stored right after its definition. In this example one value will be stored in one argument somewhere in out1, another value will be stored in another argument in out2, than they both are reloaded in codeRepl block, but PHI in exit1 (or exit2) uses only one value from region, so it should be determenistic. I completely agree that this assumption requires additional test, I'll try to provide it soon.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:635
+                                   ExitBB->getParent(), ExitBB);
+        for (pred_iterator I = pred_begin(ExitBB), E = pred_end(ExitBB);
+             I != E;) {
----------------
vsk wrote:
> Why not "for (BasicBlock &PredBB : predecessors(ExitBB))"?
The reason is that iterators become invalidated when terminator instuction in predecessor is updated


================
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))
----------------
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.


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

https://reviews.llvm.org/D55018