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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked 2 inline comments as done.
mkazantsev added inline comments.


================
Comment at: include/llvm/Analysis/MustExecute.h:57
 
-/// Computes safety information for a loop checks loop body & header for
-/// the possibility of may throw exception, it takes LoopSafetyInfo and loop as
-/// argument. Updates safety information in LoopSafetyInfo argument.
-/// Note: This is defined to clear and reinitialize an already initialized
-/// LoopSafetyInfo.  Some callers rely on this fact.
-void computeLoopSafetyInfo(LoopSafetyInfo *, Loop *);
+  void reset(Loop *L);
+
----------------
fedor.sergeev wrote:
> reames wrote:
> > Move the previous comment from computeLoopSafeyInfo to here with appropriate editing.  
> Just 'reset' seems to be somewhat misleading.
> I would still prefer to see this function having 'compute' in its name, since it does nontrivial amount of work.
> Besides everything else it does a call to colorEHFunclets which has a comment - "Consider this analysis to be expensive".
> 
It's already in cpp file.


================
Comment at: include/llvm/Analysis/MustExecute.h:66
+  // is done to this LoopSafetyInfo.
+  Loop *CurLoop = nullptr;
+};
----------------
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.


================
Comment at: lib/Analysis/MustExecute.cpp:29
+  assert(CurLoop && "Should calculate the info first!");
+  assert(CurLoop->getHeader() == BB && "Currently we only support header!");
+  return HeaderMayThrow;
----------------
reames wrote:
> Bad API.  A better adapter  implementation:
> if (getHeader() == BB)
>   return HeaderMayThrow;
> else
>  return MayThrow;
What you suggested contradicts the method's specification. It is not supposed to give false-positive answers.


https://reviews.llvm.org/D50426