[PATCH] D50426: [NFC][MustExecute] Rework API to start making better analysis
Max Kazantsev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 13 04:33:25 PDT 2018
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
More information about the llvm-commits
mailing list