[PATCH] D107534: Add a pass to garbage-collect empty basic blocks after code generation.

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added inline comments.


================
Comment at: llvm/lib/CodeGen/GCEmptyBasicBlocks.cpp:53
+    if (HasAnyRealCode)
+      continue;
+
----------------
dblaikie wrote:
> rahmanl wrote:
> > wenlei wrote:
> > > rahmanl wrote:
> > > > wenlei wrote:
> > > > > tmsriram wrote:
> > > > > > If the block has no real instructions but debug instructions, is it correct to nuke it?
> > > > > For debuggbility, probably not. Relatedly the current implementation also keep the block if it contains pseudo-probe, which is what we want. 
> > > > I am guessing debug-info instructions in a basic block with zero binary size is never used. Any ideas @dblaikie ?
> > > > 
> > > > @wenlei My initial idea was to GC all empty blocks before inserting pseudo probes. Now I realize pseudo probes may be inserted anytime. But I still think we can remove empty blocks with pseudo probes at this point of CodeGen. WDYT?
> > > pseudo-probes are inserted very early - similar to where IRPGO probes are inserted. If a block has pseudo-probe in it, we should not remove that block, because the purpose of pseudo-probe is to preserve control flow (branches) so profile can be mapped to unoptimized IR more accurately. 
> > Thanks for the explanation. I got a bit confused looking at where PseudoProbeInserter is added in TargetPassConfig.cpp, which is only for callsites.
> @jmorse for debug location intrinsic functions - but my guess would be deleting an unreachable block would be fine. I'd guess SimplifyCFG already does this? (you could try it -and if it does just delete the block, despite the presence of debug location intrinsics, it's probably a good sign you can do that too)
Here, we are deleting basic blocks with no real instructions (even if they are reachable). These basic blocks manifest in the binary as blocks which have the same begin address as their next block. So debuggers can use the debug-info from their next block instead.


================
Comment at: llvm/lib/CodeGen/GCEmptyBasicBlocks.cpp:58
+    // predecessors. Although this shouldn't happen in valid code.
+    if (NextMBB == E && !Preds.empty())
+      continue;
----------------
rahmanl wrote:
> rahmanl wrote:
> > tmsriram wrote:
> > > If it is the last block it must have predecessors or you couldn't get to it, so !Preds.empty() is redundant.
> > I found one case of a function with a single empty basic block which prompted me to add this check.
> To be more precise, if I do remove the single empty basic block in that function, the compilation later breaks (during CFI-insertion) because the function becomes empty. I will take a look to see if I can mitigate the breakage. 
Turns out my check `if (MF.size() < 2)` guards against this case already. Applied your suggestion and then refactored a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107534