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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: lib/CodeGen/CSRFIShrinkWrapInfo.cpp:106
+        // work there either.
+        if (MI.isTerminator())
+          for (const MachineBasicBlock *Succ : MBB.successors())
----------------
junbuml wrote:
> Don't we need to check all terminators in MBB, instead of checking if MI is a terminator? 
Is that really necessary? I couldn't find any way in which this approach (marking the successors) doesn't work.


================
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;
----------------
junbuml wrote:
> typo : ressources 
Sorry, I'm not sure what the issue is here?


================
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
----------------
junbuml wrote:
> Why don't we make this as a method in ShringWrapper? 
My plan is to separate the ShrinkWrapper and ShrinkWrapInfo from MachineFrameInfo, so that it can be reused easily.

One of my next steps is to move the invocation from the ShrinkWrap pass to PEI. That way, we can easily switch between CSRFIShrinkWrapInfo and something like PartialShrinkWrapInfo where we would track more than one element, so we would have more than one element to save/restore.

For something like PartialShrinkWrapInfo, we would need more than MFI.SavePoint and MFI.RestorePoint. We would need to know which registers need to be saved in which basic block, and separately, where to emit the prologue/epilogue.

So I would like to keep the "interpretation" of the results of the ShrinkWrapper separate from the algorithm itself. We could think of some way to put this in CSRFIShrinkWrapInfo, but I'm not sure how to do it in a reusable way.


https://reviews.llvm.org/D41765