[PATCH] D131824: Split EH code by default

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:86
+setDescendantEHBlocksCold(SmallVectorImpl<MachineBasicBlock *> &EHBlocks,
+                          MachineBasicBlock *StartBlock) {
+
----------------
I think it would be safer to capture MachineFunction& MF here instead of relying on the user to pass MF.front(). Then you can initialize StartBlock to MF.front() in this function.


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

e.g. Status, Unknown, EH, NonEH.


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


================
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.
----------------
Update the comment here to say that we can also split EH dominated blocks if requested.


================
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.
----------------
Can we use MachineDominatorTree::getDescendants here instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131824