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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
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
----------------
mkazantsev wrote:
> reames wrote:
> > 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.
> If we check it on non-null, it fails some unit tests. I can add add this assertion only with unit tests modification in a separate patch.
As for motivation for this, DT is here not because otherwise it would have any risky implications, but because all files where we create LoopSafetyInfo need update because we are going to use it in the future. If we can make a mass update in the main patch, there is no point in this NFC at all.


https://reviews.llvm.org/D50426