[PATCH] D131824: Split EH code by default

[Archit Saxena via Phabricator via llvm-commits]

iamarchit123 added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:88
+
+  enum status { UNKNOWN = 0, EH = 1, NONEH = 2 };
+  DenseSet<MachineBasicBlock *> WorkList;
----------------
snehasish wrote:
> enums should be InitCaps: 
> https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> 
> e.g. Status, Unknown, EH, NonEH.
Done


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:102
+      status PredStatus = getStatus(PredMBB);
+      if (PredStatus > Status)
+        Status = PredStatus;
----------------
snehasish wrote:
> Please add a comment here to describe the `PredStatus > Status` check.
Done


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:129
+
+    // get old status
+    status OldStatus = getStatus(MBB);
----------------
snehasish wrote:
> nit: Avoid comments which simply state whats going on if its clear from the code.
Removed comment


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:164
 bool MachineFunctionSplitter::runOnMachineFunction(MachineFunction &MF) {
   // TODO: We only target functions with profile data. Static information may
   // also be considered but we don't see performance improvements yet.
----------------
snehasish wrote:
> Update the comment here to say that we can also split EH dominated blocks if requested.
Updated description to include static splitting of EH code


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:213
+  if (SplitAllEHCode)
+    setDescendantEHBlocksCold(LandingPads, &MF.front());
   // We only split out eh pads if all of them are cold.
----------------
snehasish wrote:
> iamarchit123 wrote:
> > snehasish wrote:
> > > Can we use MachineDominatorTree::getDescendants here instead?
> > Hi Snehashish,
> > What about blocks that may not be dominated by single EH pad but by two EH pads? Like reachable from both EH1 and EH2. Won't we miss those blocks?
> Yes we will, this approach makes sense. Can you document this and add a descriptive function comment for setDescendantEHBlocksCold?
Added comments  for setDescendantEHBlocksCold above function definition


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131824