[PATCH] D42600: [CodeGen][Shrink-wrap]split restore point

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hey,

I only had a cursory look but the overall direction seems good.

I'll look closer after you add comments that goes into more details of what each function is doing and lay off the nomenclature around dirty, clean, and splittable.

Cheers,
-Quentin



================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:103
+                            cl::Hidden,
+                            cl::desc("enable the post shrink-wrapping"));
 
----------------
How about a more descriptive name like `enable-shrink-wrap-region-split`?


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:192
+  // Try to find safe point based on dominance and block frequency without
+  // any chance in IR.
+  bool performShrinkWrapping(MachineFunction &MF, RegScavenger *RS);
----------------
Typo: chance => change


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:199
+
+  // Return ture if current restore point is splittable.
+  bool checkIfRestoreSplittable(
----------------
Here and everywhere: use `///` to enable doxygen comments.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:200
+  // Return ture if current restore point is splittable.
+  bool checkIfRestoreSplittable(
+      MachineBasicBlock *CurRestore,
----------------
Please document the method and all the parameters.
In particular what it means to be a clean/dirty predecessor and what we check here.
I see that you have some more comments in the cpp, but I believe it doesn't answer these questions.

Also we should document what we mean by splittable with respect to what because a block should always be splittable in the more general term, but that may not help us.
I.e., we should always be able to do something like
```
BB:
   <...>
```
=>
```
BB:
   // fall through
NewBB:
   <...>
```
That's useless but there is nothing preventing us to do that so technically blocks are splittable :).


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:363
+
+static bool hasDirtyPred(DenseSet<const MachineBasicBlock *> &ReachableByDirty,
+                         MachineBasicBlock &MBB) {
----------------
Both arguments can be const


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:363
+
+static bool hasDirtyPred(DenseSet<const MachineBasicBlock *> &ReachableByDirty,
+                         MachineBasicBlock &MBB) {
----------------
qcolombet wrote:
> Both arguments can be const
Document what dirty means.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:868
+    // construction/deconstruction of the stack frame.
+    // If MF is irreducible, a block may be in a loop without
+    // MachineLoopInfo reporting it. I.e., we may use the
----------------
The comment starting at " If MF is irreducible, a block may be in a loop without" is duplicated here.


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

https://reviews.llvm.org/D42600