[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:
> 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.   
> 
> 
You're totally right, sorry. I was confused between CSRFIShrinkWrapInfo and a version which tracks multiple registers so it has to go through all the instructions.

How about iterating on the instructions in reverse order? We will reach terminators first, and if they use any CSR/FI, we can safely assume that if any non-terminator instructions use CSR/FI, the terminators have been checked first.


https://reviews.llvm.org/D41765