[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:106
+        // work there either.
+        if (MI.isTerminator())
+          for (const MachineBasicBlock *Succ : MBB.successors())
----------------
thegameg wrote:
> 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.
As far as I understand, if a block has an instruction using csr and also has a terminator using csr. Then, we have to mark its successors as used as well so that we can safely place the restore point after the terminator.  
Assuming what I understand is correct, if MI using csr is a non-terminator, then current code may not mark its successor even when MBB's terminator use csr.  When MBB is marked, we should also check if its terminators use csr before break in line 110.   




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


https://reviews.llvm.org/D41765