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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: include/llvm/Analysis/MustExecute.h:53
+
+  bool mayThrow(const BasicBlock *BB);
+
----------------
mkazantsev wrote:
> fedor.sergeev wrote:
> > reames wrote:
> > > Should be const functions.
> > This most definitely is a "preparation" for D50377, where LoopSafetyInfo gets more state and mayThrow becomes non-const.
> > 
> > Said that, I do question the need for doing a separate API change that goes w/o a semantical context that necessitates the change.
> > 
> I agree with Fedor, I would rather not do all these methods `const` because in the follow-up they all become non-const because of caching added.
These methods are semantically const.  If the internal caching is needed for efficiency, the ICF info can be marked mutable.  


================
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;
----------------
mkazantsev wrote:
> 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.
Er, I disagree.  "may" always allows a conservative answer.

A valid answer to the question "may X happen"? Is *always* yes.

(To avoid confusion: this is not normal english usage, it is specifically jargon used within the compiler field.  "may" and "must" have very specific meaning.  For examples, see may alias and must alias)


================
Comment at: lib/Analysis/MustExecute.cpp:25
 
+/// Returns true if the block \p BB contains an instruction that may throw or
+/// otherwise exit abnormally.
----------------
Style: document methods only in header, not cpp.


https://reviews.llvm.org/D50426