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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Initial pass at basic code style review.



================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:360-362
+  if (TII.analyzeBranch(Entry, TBB, FBB, Cond))
+    return false;
+  return true;
----------------
```
return !TII.analyzeBranch(...
```


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:388
+static void collectBlocksReachableByDirty(
+    MachineFunction &MF, DenseSet<const MachineBasicBlock *> &DirtyBBs,
+    DenseSet<const MachineBasicBlock *> &ReachableByDirty) {
----------------
`MF` is unused


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:401
+isSaveReachableThroughClean(const MachineBasicBlock *SavePoint,
+                            SmallVector<MachineBasicBlock *, 2> CleanPreds) {
+  DenseSet<const MachineBasicBlock *> Visited;
----------------
Use `ArrayRef` from ADT for parameters.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:416
+
+static void updateTerminator(MachineBasicBlock *BBToUpdate,
+                             MachineBasicBlock *NMBB,
----------------
Please add a comment above the definition describing what this function is doing.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:426
+
+static MachineBasicBlock *
+tryToSplitRestore(MachineBasicBlock *MBB,
----------------
Please add a comment above the definition describing what this function is doing.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:439
+  SmallPtrSet<MachineBasicBlock *, 8> MBBFallthrough;
+  for (auto *BB : DirtyPreds) {
+    if (BB->getFallThrough(false) == MBB)
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:442
+      MBBFallthrough.insert(BB);
+  }
+  for (auto *BB : CleanPreds) {
----------------
remove {} from for


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:443
+  }
+  for (auto *BB : CleanPreds) {
+    if (BB->getFallThrough(false) == MBB)
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:446
+      MBBFallthrough.insert(BB);
+  }
+
----------------
remove {} from for


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:451
+
+  for (const auto LI : MBB->liveins())
+    NMBB->addLiveIn(LI.PhysReg);
----------------
replace auto with RegisterMaskPair&


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:458
+  // blocks.
+  for (auto SuccBB : DirtyPreds) {
+    SuccBB->ReplaceUsesOfBlockWith(MBB, NMBB);
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:460
+    SuccBB->ReplaceUsesOfBlockWith(MBB, NMBB);
+  }
+  NMBB->addSuccessor(MBB);
----------------
remove for {}


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:469
+
+static void rollbackRestoreSplit(MachineFunction &MF, MachineBasicBlock *NMBB,
+                                 MachineBasicBlock *MBB,
----------------
What's `NMBB`? Maybe a more descriptive identifier might be nice?


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:479
+  SmallPtrSet<MachineBasicBlock *, 8> NMBBFallthrough;
+  for (auto *BB : DirtyPreds) {
+    if (BB->getFallThrough(false) == NMBB)
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:482
+      NMBBFallthrough.insert(BB);
+  }
+  for (auto *BB : CleanPreds) {
----------------
remove for {}


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:483
+  }
+  for (auto *BB : CleanPreds) {
+    if (BB->getFallThrough(false) == NMBB)
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:486
+      NMBBFallthrough.insert(BB);
+  }
+
----------------
remove for {}


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:489
+  NMBB->removeSuccessor(MBB);
+  for (auto SuccBB : DirtyPreds) {
+    SuccBB->ReplaceUsesOfBlockWith(NMBB, MBB);
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:491
+    SuccBB->ReplaceUsesOfBlockWith(NMBB, MBB);
+  }
+
----------------
remove for {}


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:496
+
+  for (auto *BBToUpdate : NMBBFallthrough)
+    updateTerminator(BBToUpdate, MBB, TII);
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:500
+
+bool ShrinkWrap::checkIfRestoreSplittable(
+    MachineBasicBlock *CurRestore,
----------------
Please add a comment above the definition describing what this function is doing.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:510
+
+  for (auto PredBB : CurRestore->predecessors()) {
+    if (!isAnalyzableBB(*TII, *PredBB))
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:556
+  DenseSet<const MachineBasicBlock *> DirtyBBs;
+  for (auto &MBB : MF) {
+    if (MBB.isEHPad()) {
----------------
remove auto


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:579
+
+  // Trying to reach out to the new save point which dominate all dirty blocks.
+  MachineBasicBlock *NewSave =
----------------
dominates


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:884-885
+    return false;
+  if (!ArePointsInteresting())
+    return false;
 
----------------
should we still return `false` here if if `Changed == true`?

Am I reading this right? Did the previous version of `ShrinkWrap::runOnMachineFunction` never return `true`?


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

https://reviews.llvm.org/D42600