[PATCH] D41765: [CodeGen] Provide an advanced shrink-wrapping interface

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/CodeGen/CSRFIShrinkWrapInfo.cpp:102-103
+        // Make sure we would be able to insert the restore code before the
+        // terminator. Mark the successors as used as well, since we can't save
+        // after a terminator (i.e. cbz w23, #10). This doesn't handle the case
+        // when a return instruction uses a CSR. The default placement won't
----------------
Little confusing about this comment.  Should it be like :  since we cannot restore before a terminator using CSR? 


================
Comment at: lib/CodeGen/CSRFIShrinkWrapInfo.cpp:106
+        // work there either.
+        if (MI.isTerminator())
+          for (const MachineBasicBlock *Succ : MBB.successors())
----------------
Don't we need to check all terminators in MBB, instead of checking if MI is a terminator? 


================
Comment at: lib/CodeGen/CSRFIShrinkWrapInfo.h:56
+  const SetOfRegs &getCurrentCSRs(RegScavenger *RS) const;
+  /// Return true if MI uses any of the ressources that we are interested in.
+  bool useOrDefCSROrFI(const MachineInstr &MI, RegScavenger *RS) const;
----------------
typo : ressources 


================
Comment at: lib/CodeGen/DominatorShrinkWrapper.cpp:212
+  for (MachineBasicBlock &MBB : MF) {
+    if (Tracking.none())
+      return;
----------------
what about something like hasElementToTrack() ?


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:107-126
+  const BitVector &TrackedElts = SW.getTrackedElts();
+  if (!TrackedElts.test(CSRFIShrinkWrapInfo::CSRFIUsedBit))
     return false;
-  }
 
-  DEBUG(dbgs() << "\n ** Results **\nFrequency of the Entry: " << EntryFreq
-               << '\n');
-
-  const TargetFrameLowering *TFI = MF.getSubtarget().getFrameLowering();
-  do {
-    DEBUG(dbgs() << "Shrink wrap candidates (#, Name, Freq):\nSave: "
-                 << Save->getNumber() << ' ' << Save->getName() << ' '
-                 << MBFI->getBlockFreq(Save).getFrequency() << "\nRestore: "
-                 << Restore->getNumber() << ' ' << Restore->getName() << ' '
-                 << MBFI->getBlockFreq(Restore).getFrequency() << '\n');
+  // If the CSRFIUsedBit is set, we can now assume that:
+  // * All used elements are tracked (only one in this case)
+  // * There is *exactly one* save
----------------
Why don't we make this as a method in ShringWrapper? 


https://reviews.llvm.org/D41765