[PATCH] D50426: [NFC][MustExecute] Rework API to start making better analysis

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added inline comments.


================
Comment at: include/llvm/Analysis/MustExecute.h:66
+  // is done to this LoopSafetyInfo.
+  Loop *CurLoop = nullptr;
+};
----------------
mkazantsev wrote:
> reames wrote:
> > Not sure it's safe to keep a raw reference here.  The loop pass manager can delete loops in some cases.  I'd suggest splitting this out into it's own patch for ease of review and revert.
> Whoever removes a loop should stop using its safety info, otherwise it makes no sense. Currently, most use-cases of this is just create + call throws check with in place, except for loop unswitching which makes reset for every use. So I see no reason why it would be mo dangerous than keeping track of loop blocks in ICF or SCEV.
> 
> I also doubt that it would be easily revertable: the patch on top of that touches methods for which it will change signatures.
> Whoever removes a loop should stop using its safety info
Alternative would be to always take Loop as a parameter to all query functions (similar to how it is passed to isGuaranteedToExecute) and use CurLoop only for something like:
   assert(CurLoop == Loop && "this is a wrong loop")

To me, ideal scheme for LoopSafetyInfo would be to make it part of Loop, effectively converting it into a transparent helper cache for queries to Loop like isGuaranteedToExecute rather than something that should be managed separately by users of those queries.

> why it would be more dangerous than keeping track of loop blocks in ICF
To me ICF's manual handling of invalidateBlock **is** dangerous.
Caching might be the only way to get a sane compilation time, however trading compile-time for correctness is always a scary trade-off. See my rants in D50377.

> except for loop unswitching which makes reset for every use
And I dont see anything in loop unswitching code that specifically requires this particular reset pattern.
We could make LoopSafetyInfo local to processCurrentLoop without any visible downsides and providing a benefit of updated SafetyInfo on a subsequent redoLoop.


https://reviews.llvm.org/D50426