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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Analysis/MustExecute.h:44
 
-  LoopSafetyInfo() = default;
-};
+  LoopSafetyInfo(Loop *L, DominatorTree *DT) {
+    // TODO: DT will later be used by ICF analysis that will come in a follow-up
----------------
Drop the DT from this patch.  That wasn't the API change with the risky implications.

Also, shouldn't L always be non-null?  If so, assert or use reference.  

Reading down in the patch, I see that L can be null.  You'd be better with two constructors here.  One which explicitly created an uninitialized object and the other which always created an initialized one.


================
Comment at: include/llvm/Analysis/MustExecute.h:53
+
+  bool mayThrow(const BasicBlock *BB);
+
----------------
Should be const functions.


================
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);
+
----------------
Move the previous comment from computeLoopSafeyInfo to here with appropriate editing.  


================
Comment at: include/llvm/Analysis/MustExecute.h:59
+
+private:
+  bool MayThrow = false;       // The current loop contains an instruction which
----------------
It's idiomatic to put private variables at the top of the class def, not the bottom.   In this particular case, it'll reduce the textual diff as well.


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


================
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;
----------------
Bad API.  A better adapter  implementation:
if (getHeader() == BB)
  return HeaderMayThrow;
else
 return MayThrow;


================
Comment at: lib/Analysis/MustExecute.cpp:120
                                  const DominatorTree *DT, const Loop *CurLoop,
-                                 const LoopSafetyInfo *SafetyInfo) {
+                                 LoopSafetyInfo *SafetyInfo) {
   // We have to check to make sure that the instruction dominates all
----------------
Why drop the const?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:97
 static bool hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
-                  const LoopSafetyInfo *SafetyInfo,
                   OptimizationRemarkEmitter *ORE);
----------------
If you need to drop const everywhere, you need to revisit your API.  (clue: you don't)


https://reviews.llvm.org/D50426