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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Please mark comments as "Done" in phabricator so that it's clear to reviewers which comment threads are still outstanding.



================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:210-211
+      const DenseSet<const MachineBasicBlock *> &ReachableByDirty,
+      SmallVector<MachineBasicBlock *, 2> &DirtyPreds,
+      SmallVector<MachineBasicBlock *, 2> &CleanPreds,
+      const TargetInstrInfo *TII, RegScavenger *RS);
----------------
I think it's more appropriate to pass a `SmallVectorImpl<MachineBasicBlock*>&` rather than `SmallVector<MachineBasicBlock *, 2>&` (having the parameter specialization of `2` is...overly specific). It would be preferable to use `ArrayRef`, but you can't `push_back` an `ArrayRef`, so the `SmallVectorImp<>&` is the way to go.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:451
+///
+/// Decision has been made to split the restore point.
+/// old restore point: \p MBB
----------------
Seems like it can be undone though, IIUC?


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:460
+                  DenseSet<const MachineBasicBlock *> &ReachableByDirty,
+                  SmallVector<MachineBasicBlock *, 2> &DirtyPreds,
+                  const TargetInstrInfo *TII) {
----------------
ArrayRef


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:503
+                     MachineBasicBlock *MBB,
+                     SmallVector<MachineBasicBlock *, 2> &DirtyPreds,
+                     const TargetInstrInfo *TII) {
----------------
ArrayRef


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:531-532
+    const DenseSet<const MachineBasicBlock *> &ReachableByDirty,
+    SmallVector<MachineBasicBlock *, 2> &DirtyPreds,
+    SmallVector<MachineBasicBlock *, 2> &CleanPreds, const TargetInstrInfo *TII,
+    RegScavenger *RS) {
----------------
SmallVectorImpl<MachineBasicBlock *>&


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


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


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:884-885
+    return false;
+  if (!ArePointsInteresting())
+    return false;
 
----------------
qcolombet wrote:
> sushgokh wrote:
> > thegameg wrote:
> > > nickdesaulniers wrote:
> > > > 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`?
> > > Correct, the previous version doesn't change the code, it just tells `PrologEpilogInserter` where to place the prologue/epilogue through `MachineFrameInfo`.
> > > 
> > > I guess here we should return true if we do any edge-splitting.
> > @nickdesaulniers I think no need of returning true if Changed == true because dominance/post-dominance relations are updated after post-shrinking. Is there any other thing that needs updation ?
> If we changed the CFG (or anything) we need to return true. There may be other things that we are destroying without realizing (e.g., live intervals, but that's not applicable here. The point is we don't know which analysis rely on what.)
`tryToSplitRestore` is modifying the CFG, right? If that happens, we MUST return `true`.

Should we perhaps skip calling `postShrinkWrapping` unless `HasCandidate` is `true`?


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

https://reviews.llvm.org/D42600